feat: implement batch map #133
Conversation
pkg/batchmapper/service.go
Outdated
| if len(responses.Items()) != len(datums) { | ||
| errMsg := "batchMapFn: mismatch between length of batch requests and responses" | ||
| log.Panic(errMsg) | ||
| } |
There was a problem hiding this comment.
is it possible to avoid panic?
There was a problem hiding this comment.
We can choose either to panic or send an error back(numa will be restarted in both cases), based on an earlier discussion with @vigith we thought that having a panic here would be a clear indication for the user that there is an issue. And as this in a mismatch in the implementation from the user side for not providing equal number of responses, the reason for restarts and error gives better clarity there.
But this is our decision on how do we want to keep the behavior in such scenarios.
@vigith @whynowy Any preferences?
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: a3hadi <[email protected]> Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
| func (u *BatchMapFnServerErrTest) Context() context.Context { | ||
| return u.ctx | ||
| } | ||
|
|
||
| func TestService_BatchMapFn(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| handler BatchMapper | ||
| input []*batchmappb.BatchMapRequest | ||
| expected []*batchmappb.BatchMapResponse | ||
| expectedErr bool | ||
| }{ |
There was a problem hiding this comment.
Can we add a test to cover the error/panic case inside the user code?
There was a problem hiding this comment.
In the current setup, this is difficult to test via UT directly. I tried to add the test there but the assert.Panics (which internally uses recover) was not able to catch it as the panic is invoked from within another goroutine. Would need to research more on how can we cover something like this. In python i'm able to mock the panic and test which wasn't working out here.
I tried few manual scenarios to induce such panics which were working fine.
Fixes #1786