[BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java SDK. by ajamato · Pull Request #14805 · apache/beam

ajamato · 2021-05-13T03:43:04Z

[BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java SDK.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Samza	Twister2
Go	---	---	---
Java
Python	---	---	---
XLang		---	---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

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.

ajamato · 2021-05-13T03:52:44Z

R: @robertwb @pabloem

Split from
#14490

aaltay · 2021-05-27T22:53:36Z

@robertwb @pabloem: Would you be able to review this?

ajamato · 2021-05-28T01:38:12Z

Not waiting for review, discussed offline. Need to make some changes. Will get to it in in about 3 weeks.

ajamato · 2021-06-16T00:18:57Z

R: @robertwb R: @pabloem

Hi, lets pick up this review again. I removed all reference to startTimes, no longer using these on the MonitoringInfo.

pabloem · 2021-06-18T20:53:17Z

LGTM.

@robertwb any comments?

robertwb · 2021-06-18T22:32:58Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

Is not being bound to a step equivalent to being process wide?

No. its not equivalent. We have these in a some other places I believe.

It was used here as well

beam/runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerStepMap.java

Line 52 in 8463a05

private MetricsContainerImpl unboundContainer = new MetricsContainerImpl(null);

I believe this was added for some metrics which had the step already in the label information already. (In some code which was tracking metrics for multiple steps). So there was no need to have the container know about the step name.

Likely one of the system metrics. ElementCount, ExecutionTime, etc.

If you feel this needs to be changed, please pick a specific option and I will introduce it. One I can think of is:
adding static factory method (which will set the step to null and isProcessWide to true):
public static MetricsContainerImpl createProcessWideContainer();

It feels like that if MetricContainer[Impls] are only sometimes associated with steps, the step association should be stored elsewhere, not as part of this instance. But maybe that's too big a refactoring for now.

+1 to MetricsContainerImpl.createProcessWideContainer(); the nullary constructor doesn't make it obvious the intent is to have this process-wide.

Added MetricsContainerImpl.createProcessWideContainer()

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

robertwb · 2021-06-18T22:34:47Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

Is the only use of this to prevent it from being reset? (Which, presumably, would be easy to accidentally do?) If so, can we make this private (or just access the member directly) and remove the addition to the interface?

Done. Made it private

(Yes. Previously there was a PR which had called reset() on it which caused some debugging issued when adding this cl.)

robertwb · 2021-06-18T22:37:53Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java

Remind me why MetricsEnvironment needs a setter rather than it just implicitly always creating one.

The main reason was to allow clearing/cleaning the state between tests.

Explicitly creating it forces you to create a clean state on startup.
We could have had one here by default as well. I don't feel too strongly here.
Though I would consider it out of scope of this PR

My preference is to have the tests be non-idiomatic (e.g. you could have a reset-process-container-for-test method) than have the "production" code be non-idiomatic to accommodate tests. Having a setter doesn't prevent state from leaking from test to test (though I guess it forces you to set it in at least one test).

Okay, I looked into this a bit more. And I don't think that I can set the MetricContainerInmpl as the default container in MetricsEnvironment without adding a bad dependency, which I think may introduce a cycle.

As intellij seems to want to add the dependency when I try to instantiate the MetricsContainerImpl in MetricsEnvironment.
I dont think we can add a dep in this direction. (Today, MetricsEnvironment only depends on the interface MetricsContainer).

sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/MetricsEnvironment.java
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

That is, we don't make the sdk/java/core depend on the runners/core-java.
The way it works today is because we allow sdks/java/harness to depend on runners/core-java.

which is why we can instantiate it in
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java

Some larger refactor may make this possible, but I feel its unclear what would be the best choice here and would be too big a refactor for this PR

OK, I agree a refactor seems out of scope for this PR.

ajamato · 2021-06-22T00:05:50Z

Run Java PreCommit

ajamato · 2021-06-22T00:06:11Z

Precommit is flakey, but lets not let that block the review at the moment

ajamato · 2021-06-23T03:52:47Z

Run Java PreCommit

aaltay · 2021-06-29T04:09:19Z

All tests are passing. Is this ready to be merged?

…quest and MonitoringInfosMetadataRequest/Response support to Java SDK.

ajamato · 2021-06-30T19:28:44Z

Run Java PreCommit

ajamato · 2021-06-30T19:28:47Z

Run Java_Examples_Dataflow PreCommit

ajamato · 2021-07-02T17:24:30Z

Thanks @aaltay @robertwb
Would you please merge?

ajamato mentioned this pull request May 13, 2021

DO NOT SUBMIT SPLIT INTO OTHER PRsAdd HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java #14490

Closed

4 tasks

ajamato force-pushed the bq_metrics_process_wide_process_wide branch from e2fb2f2 to 3c89635 Compare May 13, 2021 03:47

ajamato force-pushed the bq_metrics_process_wide_process_wide branch 4 times, most recently from bbe509f to 2933b02 Compare June 15, 2021 20:18

robertwb reviewed Jun 18, 2021

View reviewed changes

ajamato force-pushed the bq_metrics_process_wide_process_wide branch 2 times, most recently from 9988de8 to 02a40a3 Compare June 21, 2021 22:03

[BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRe…

48ad919

…quest and MonitoringInfosMetadataRequest/Response support to Java SDK.

ajamato force-pushed the bq_metrics_process_wide_process_wide branch from 02a40a3 to 48ad919 Compare June 30, 2021 01:39

robertwb approved these changes Jul 2, 2021

View reviewed changes

pabloem merged commit a60aeba into apache:master Jul 2, 2021

Conversation

ajamato commented May 13, 2021

ValidatesRunner compliance 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)

GitHub Actions Tests Status (on master branch)

Uh oh!

ajamato commented May 13, 2021

Uh oh!

aaltay commented May 27, 2021

Uh oh!

ajamato commented May 28, 2021

Uh oh!

ajamato commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pabloem commented Jun 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajamato commented Jun 22, 2021

Uh oh!

ajamato commented Jun 22, 2021

Uh oh!

ajamato commented Jun 23, 2021

Uh oh!

aaltay commented Jun 29, 2021

Uh oh!

ajamato commented Jun 30, 2021

Uh oh!

ajamato commented Jun 30, 2021

Uh oh!

ajamato commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`ValidatesRunner` compliance status (on master branch)

ajamato commented Jun 16, 2021 •

edited

Loading