Support for requestPays in java-storage call flow #995
Conversation
|
/gcbrun |
singhravidutt
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #995 +/- ##
============================================
+ Coverage 76.73% 82.39% +5.65%
- Complexity 1640 1782 +142
============================================
Files 101 101
Lines 7935 7946 +11
Branches 990 992 +2
============================================
+ Hits 6089 6547 +458
+ Misses 1409 1002 -407
+ Partials 437 397 -40
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| @@ -51,7 +51,7 @@ | |||
| @VisibleForTesting | |||
| public class GoogleCloudStorageClientImpl extends ForwardingGoogleCloudStorage { | |||
| private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); | |||
|
|
|||
| private static GoogleCloudStorageImpl delegate; | |||
| @@ -51,7 +51,7 @@ | |||
| @VisibleForTesting | |||
| public class GoogleCloudStorageClientImpl extends ForwardingGoogleCloudStorage { | |||
| private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); | |||
|
|
|||
| private static GoogleCloudStorageImpl delegate; | |||
There was a problem hiding this comment.
Why is this static?
| @@ -51,7 +51,7 @@ | |||
| @VisibleForTesting | |||
| public class GoogleCloudStorageClientImpl extends ForwardingGoogleCloudStorage { | |||
| private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); | |||
|
|
|||
| private static GoogleCloudStorageImpl delegate; | |||
There was a problem hiding this comment.
Having the base class and derived class having same field is an anti pattern
| @@ -160,6 +168,11 @@ public void close() { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public GoogleCloudStorageImpl getDelegate() { | |||
There was a problem hiding this comment.
this is overriding the same method as base class?
|
|
||
| public GoogleCloudStorageClientWriteChannel( | ||
| Storage storage, | ||
| GoogleCloudStorageOptions storageOptions, | ||
| StorageResourceId resourceId, | ||
| CreateObjectOptions createOptions, | ||
| boolean requesterPays, |
There was a problem hiding this comment.
wont this break the public API? Maybe add a new constructor?
|
|
||
| public GoogleCloudStorageClientWriteChannel( | ||
| Storage storage, | ||
| GoogleCloudStorageOptions storageOptions, | ||
| StorageResourceId resourceId, | ||
| CreateObjectOptions createOptions, | ||
| boolean requesterPays, |
There was a problem hiding this comment.
consider renaming this to something like requesterPaysEnabled or something similar
| readChannel = getJavaStorageChannel(DEFAULT_ITEM_INFO, DEFAULT_READ_OPTION); | ||
| } | ||
|
|
||
| @Test | ||
| public void verifyRequesterPaysOption() throws IOException { |
There was a problem hiding this comment.
Do we have any integration test for requesterPays?
| fakeReadChannel = spy(new FakeReadChannel(CONTENT)); | ||
| when(mockedStorage.reader(blobIdCaptor.capture(), blobSourceOptionCaptor.capture())) | ||
| .thenReturn(fakeReadChannel); | ||
| readChannel = |
There was a problem hiding this comment.
what are we really testing here?
No description provided.