Fix write inconsistency in the gRPC implementation #990
Conversation
|
/gcbrun |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #990 +/- ##
============================================
+ Coverage 76.73% 82.44% +5.71%
- Complexity 1640 1796 +156
============================================
Files 101 102 +1
Lines 7935 8027 +92
Branches 990 996 +6
============================================
+ Hits 6089 6618 +529
+ Misses 1409 1009 -400
+ Partials 437 400 -37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| : false; | ||
| } | ||
|
|
||
| public WriteObjectResponse closeStream() throws IOException { |
There was a problem hiding this comment.
Do we want this class to implement closeable java interface?
| protected static final ImmutableSet<Code> TRANSIENT_ERRORS = | ||
| ImmutableSet.of( | ||
| Status.Code.DEADLINE_EXCEEDED, | ||
| Status.Code.INTERNAL, |
There was a problem hiding this comment.
should we really retry in this case?
|
|
||
| // Wait for streaming RPC to become ready for upload. | ||
| // wait for 1 min for the channel to be ready. Else bail out | ||
| if (!responseObserver.ready.await(60 * 1000, MILLISECONDS)) { |
There was a problem hiding this comment.
why one minute? Should we make this configurable.
| channelOptions.getGrpcWriteMessageTimeout())); | ||
|
|
||
| } catch (IOException e) { | ||
| throw e; |
| } | ||
| } | ||
|
|
||
| public void writeChunk(WriteObjectRequest request) throws IOException { |
There was a problem hiding this comment.
why this need to be public?
| if (TRANSIENT_ERRORS.contains(statusCode)) { | ||
| transientError = t; | ||
| } | ||
| if (transientError == null) { |
| } | ||
|
|
||
| public boolean isComplete() { | ||
| return done.getCount() == 0 ? true : false; |
| } | ||
|
|
||
| public boolean isReady() { | ||
| return ready.getCount() == 0 ? true : false; |
| } | ||
|
|
||
| public boolean isComplete() { | ||
| return done.getCount() < 1 ? true : false; |
|
|
||
| @Before | ||
| public void setUp() throws Exception { | ||
| MockitoAnnotations.initMocks(this); |
There was a problem hiding this comment.
We avoid using mocks in connector. Do we really need to use mocks?
medb
changed the title
Refactored gRPC write channel. Major changes
GoogleCloudStorageContentWriteStreamwhich manages a single writeObject stream.GoogleCloudStorageContentWriteStreamTestto support more usecases likeGoogleCloudStorageContentWriteStreamto test the various behaviour of resumable writeObject stream.