[BEAM-670] BigQueryServicesImpl: fix issues in insertAll and add better tests #1018
Conversation
| * Tests that {@link DatasetServiceImpl#insertAll} fails gracefully when persistent issues. | ||
| */ | ||
| @Test | ||
| public void testInsertFailsGracefully() throws Exception { |
There was a problem hiding this comment.
this is the test that catches the issue with reusing the backoff multiple times instead of using the cached nextBackoffMillis.
| } | ||
| try { | ||
| Thread.sleep(backoff.nextBackOffMillis()); | ||
| Thread.sleep(nextBackoffMillis); |
There was a problem hiding this comment.
Pass in a sleeper so that the tests don't actually call Thread.sleep
| when(response.getContentType()).thenReturn(Json.MEDIA_TYPE); | ||
| when(response.getStatusCode()).thenReturn(200).thenReturn(200); | ||
| when(response.getContent()) | ||
| .thenAnswer(new Answer<InputStream>() { |
There was a problem hiding this comment.
You can chain thenReturn calls and verify response.getContent is only called twice.
So:
thenReturn(toStream(bFailed).thenReturn(toStream(allRowsSucceeded));
| expectedLogs.verifyInfo("Retrying 1 failed inserts to BigQuery"); | ||
| } | ||
|
|
||
| // A very-short BackOff to use in tests. |
There was a problem hiding this comment.
Really, you want to mock/fake out the sleeper, check out FastNanoClockAndSleeper
There was a problem hiding this comment.
done using MockSleeper -- FastNanoClockAndSleeper seems overkill
|
|
||
| when(response.getContentType()).thenReturn(Json.MEDIA_TYPE); | ||
| when(response.getStatusCode()).thenReturn(200); | ||
| when(response.getContent()).thenAnswer(new Answer<InputStream>() { |
There was a problem hiding this comment.
ditto on using chained thenReturn
|
PTAL |
| // Return row 1 failing, then we retry row 1 as row 0, and row 0 persistently fails. | ||
| when(response.getContent()) | ||
| .thenReturn(toStream(row1Failed)) | ||
| .thenReturn(toStream(row0Failed)) |
There was a problem hiding this comment.
Whatever the last thenReturn is used will be used for all subsequent calls so no point in repeating .thenReturn(toStream(row0Failed)) 3 times.
There was a problem hiding this comment.
InputStream is not reusable, so can't reuse last value returned.
There was a problem hiding this comment.
As discussed, the multiple thenReturns are because the input is consumed since its an InputStream
| verify(response, times(2)).getStatusCode(); | ||
| verify(response, times(2)).getContent(); | ||
| verify(response, times(2)).getContentType(); | ||
| expectedLogs.verifyInfo("Retrying 1 failed inserts to BigQuery"); |
There was a problem hiding this comment.
Ah you are verifying this through log message. I was trying to verify this through a mock of the BQ client req/response and that was not as easy.
|
LGTM |
|
Travis is a flake (3/4 passed, 4th went into never land), Jenkins passed. Merging. |
| return toStream(row0Failed); | ||
| } | ||
| }); | ||
|
|
3 participants
R: @lukecwik
CC: @vikkyrk