-
Couldn't load subscription status.
- Fork 13.7k
[FLINK-38225] Sets proper precondition to ensure that GCS operations are retryable #27101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| BlobInfo blobInfo = BlobInfo.newBuilder(blobIdentifier.getBlobId()).build(); | ||
| com.google.cloud.WriteChannel writeChannel = storage.writer(blobInfo); | ||
| com.google.cloud.WriteChannel writeChannel = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to junit this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can wrap the storage in the UT and ensure that proper option is passed.
For this we need to extend the Storage. Which is for InternalExtensionOnly.
Some e2e would be great to have, but from what I understand only Azure is available in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried briefly to make some UT and without mocking it is hard. Currently GSBlobStorageImpl is too low level and it is the component which is being mocked in the codebase.
Creating a TestStorage to catch GCP calls is also hard, notable because some of the methods returns Blob object, which cannot be created without reflections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidradl thanks for challenging. I have reconsidered approach and added some tests, for functionality. As result the new test dependency to emulate GS Storage is added.
This test dependency doesn't allow to use versioning, so I cannot test the atomicity of the operations, meanwhile happy paths are added to test write and compose methods.
What is the purpose of the change
Ensures that the operations become idempotent or atomic, allowing the GCS client to safely retry the 503 errors. Thus client can actually perform the retry in such conditions.
Original retry logic was added in #24753.
Thanks
Xi Zhangfor pointing to the problem and providing the solution in https://issues.apache.org/jira/browse/FLINK-38225.Brief change log
Verifying this change
This change is already covered by existing tests, such as GSRecoverableWriterTest
Does this pull request potentially affect one of the following parts:
Documentation