[BEAM-11980] Java GCS - Implement IO Request Count metrics #15394
Conversation
benEng
changed the title
|
R: @ajamato. Would you help me to review this? |
| return channel; | ||
| } catch (IOException e) { | ||
| if (e.getCause() instanceof GoogleJsonResponseException) { | ||
| serviceCallMetric.call(((GoogleJsonResponseException) e.getCause()).getDetails().getCode()); |
There was a problem hiding this comment.
Has this been verified to be the proper way to catch this using an integration test?
There was a problem hiding this comment.
I forced an error in the create method with an invalid bucket name, so it throws an error that can be handled like this.
| HashMap<String, String> baseLabels = new HashMap<>(); | ||
| baseLabels.put(MonitoringInfoConstants.Labels.PTRANSFORM, ""); | ||
| baseLabels.put(MonitoringInfoConstants.Labels.SERVICE, "Storage"); | ||
| baseLabels.put(MonitoringInfoConstants.Labels.METHOD, "Objects.get"); |
There was a problem hiding this comment.
Lets call this something more general, incase the underlying API changes.
Then upstream code handling the MonitoringInfo won't need to change, as the Method label will not need to change.
METHOD = "GcsGet"
...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java
Outdated
Show resolved
Hide resolved
|
Run Portable_Python PreCommit |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
|
Run Java PreCommit |
|
@aaltay can you merge? |
|
Thank you all! |
| * @return a SeekableByteChannel that can read the object data | ||
| */ | ||
| @VisibleForTesting | ||
| SeekableByteChannel open(GcsPath path, GoogleCloudStorageReadOptions readOptions) |
There was a problem hiding this comment.
@chamikaramj While randomly reading this code, I was wondering if this method is the one called in pipelines or the above version with 1 argument (line 445).
edit: is this method only used in tests?
There was a problem hiding this comment.
I stumbled upon this code while working on GCS migration and had the same question...
Looks like the only references are in the tests.
There was a problem hiding this comment.
I think we will need to add the logic to the code path of reading.
Btw, writing looks fine. The count metrics are implemented in the common code path:
6 participants
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.