Skip to content

[BEAM-14116] Catch MonitoringInfoMetricName null keys or values in the constructor - #17094

Merged
lukecwik merged 7 commits into
apache:masterfrom
dpcollins-google:patch-4
Mar 23, 2022
Merged

[BEAM-14116] Catch MonitoringInfoMetricName null keys or values in the constructor#17094
lukecwik merged 7 commits into
apache:masterfrom
dpcollins-google:patch-4

Conversation

@dpcollins-google

@dpcollins-googledpcollins-google commented Mar 15, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Also fix possibly null field

@dpcollins-google

dpcollins-google commented Mar 15, 2022

Copy link
Copy Markdown
ContributorAuthor

R: @lukecwik

@dpcollins-google

dpcollins-google commented Mar 15, 2022

Copy link
Copy Markdown
ContributorAuthor

R: @scwhittle

@dpcollins-google

dpcollins-google commented Mar 16, 2022

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@dpcollins-googledpcollins-google changed the title Catch MonitoringInfoMetricName null keys or values in the constructor[BEAM-14116] Catch MonitoringInfoMetricName null keys or values in the constructorMar 16, 2022
@dpcollins-google

dpcollins-google commented Mar 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

1 similar comment
@dpcollins-google

dpcollins-google commented Mar 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

lukecwik
lukecwik requested changes Mar 17, 2022
View reviewed changes
Comment thread...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java Outdated
Comment thread...s/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoMetricName.java Outdated
Comment thread...s/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoMetricName.java Outdated
Comment thread...s/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoMetricName.java Outdated
@lukecwik

lukecwik commented Mar 17, 2022

Copy link
Copy Markdown
Member

CC: @robertwb

lukecwik
lukecwik approved these changes Mar 21, 2022
View reviewed changes

@lukecwiklukecwik left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this as is once tests are green since it is an improvement but if you do fix the nit that would be great.

Comment thread...s/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoMetricName.java Outdated

@Override
public String getName() {
if (labels.containsKey(MonitoringInfoConstants.Labels.NAME)) {

@lukecwiklukecwikMar 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ditto on not looking in the map multiple times

@lukecwik

lukecwik commented Mar 21, 2022

Copy link
Copy Markdown
Member

Run Java PreCommit

@lukecwik

lukecwik commented Mar 21, 2022

Copy link
Copy Markdown
Member

Run Java_Examples_Dataflow_Java11 PreCommit

@lukecwik

lukecwik commented Mar 21, 2022

Copy link
Copy Markdown
Member

Run Portable_Python PreCommit

@lukecwik

lukecwik commented Mar 22, 2022

Copy link
Copy Markdown
Member

Run Java PreCommit

2 similar comments
@lukecwik

lukecwik commented Mar 22, 2022

Copy link
Copy Markdown
Member

Run Java PreCommit

@lukecwik

lukecwik commented Mar 22, 2022

Copy link
Copy Markdown
Member

Run Java PreCommit

@dpcollins-google

dpcollins-google commented Mar 22, 2022

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@lukecwik

lukecwik commented Mar 22, 2022

Copy link
Copy Markdown
Member

Manually kicked off Java PreCommit for this PR https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/21736 because the github comments aren't making it to Jenkins

@lukecwik

lukecwik commented Mar 22, 2022

Copy link
Copy Markdown
Member

Run Java PreCommit

@lukecwik

lukecwik commented Mar 23, 2022

Copy link
Copy Markdown
Member

Java PreCommit finally passed

@lukecwik
lukecwik merged commit 836f82e into apache:masterMar 23, 2022
@lukecwik

lukecwik commented Mar 25, 2022

Copy link
Copy Markdown
Member

@youngoli It looks like this change uncovered additional cases. We thought we caught the cases where this was happening but additional integration testing within Google caught that this fails for Datastore as well. Filed https://issues.apache.org/jira/browse/BEAM-14179

@dpcollins-googledpcollins-google mentioned this pull request Mar 31, 2022
Merged
4 tasks
@nielmnielm mentioned this pull request May 4, 2022
Merged
nielm added a commit to nielm/beam that referenced this pull request May 4, 2022
@nielm
[BEAM-14405] Fix NPE when ProjectID is not specified in a template ex…
c6962d4
…ecution
PR apache#16547 added handling for null ProjectIDs on command line execution,
but did not handle the possibility of null ProjectIDs in template
execution. PR apache#17094 added nullness checks in MonitoringInfoMetricNames,
which then triggered NPEs if the ProjectID was not specified during a
template execution.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scwhittlescwhittlescwhittle left review comments
@lukecwiklukecwiklukecwik approved these changes

Assignees

No one assigned

Labels

coreextensionsgoogle-cloud-platform-corejavarunners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dpcollins-google@lukecwik@scwhittle