You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/11/29 11:52:49 UTC
[GitHub] [beam] rHermes opened a new pull request #13435: Remove unused endOfStreamMonitor from nexmark
rHermes opened a new pull request #13435:
URL: https://github.com/apache/beam/pull/13435
The `endOfStreamMonitor` in the `NexmarkQuery` class is not used. This PR removes it.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [X] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [X] 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.
- [X] Update `CHANGES.md` with noteworthy changes.
- [X] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam
.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-744370870
Run Direct Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735382003
The spotless check failed with a file I have not edited :confused: I can apply the spotless fix and upload that here if you want?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735381707
R: @echauchot
R: @aromanenko-dev
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-747296074
> @rHermes yes general good practice is to separate PRs that deal with different subjects. But for cleaning PRs, it is the same subject. There a lot of leftovers in nexmark what I would like to avoid is tens of PRs that remove only a couple of fields.
Got it :+1:
I've addressed your comments from the review, and rebased on top of master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on a change in pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on a change in pull request #13435:
URL: https://github.com/apache/beam/pull/13435#discussion_r535078859
##########
File path: runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/External.java
##########
@@ -78,7 +78,7 @@
private static AtomicInteger namespaceCounter = new AtomicInteger(0);
private static final ExpansionServiceClientFactory DEFAULT =
- DefaultExpansionServiceClientFactory.create(
+ DefaultExpansionServiceClientFactory.create(
Review comment:
please revert this change as it is unrelated
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/queries/NexmarkQuery.java
##########
@@ -39,7 +39,6 @@
final NexmarkConfiguration configuration;
public final Monitor<Event> eventMonitor;
public final Monitor<T> resultMonitor;
- private final Monitor<Event> endOfStreamMonitor;
Review comment:
+1 on cleaning
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Monitor.java
##########
@@ -39,20 +39,15 @@
private class MonitorDoFn extends DoFn<T, T> {
final Counter elementCounter = Metrics.counter(name, prefix + ".elements");
final Counter bytesCounter = Metrics.counter(name, prefix + ".bytes");
- final Distribution startTime = Metrics.distribution(name, prefix + ".startTime");
- final Distribution endTime = Metrics.distribution(name, prefix + ".endTime");
- final Distribution startTimestamp = Metrics.distribution(name, prefix + ".startTimestamp");
- final Distribution endTimestamp = Metrics.distribution(name, prefix + ".endTimestamp");
+ final Distribution systemTimestamp = Metrics.distribution(name, prefix + ".systemTimestamp");
Review comment:
it is not a timeStamp, it is in processing time (the wall clock) as opposed to a timestamp which is a component of an element put by the source. Please rename to "processingTime"
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/NexmarkLauncher.java
##########
@@ -336,7 +336,7 @@ private NexmarkPerf currentPerf(
perf.startupDelaySec = (eventStart - startMsSinceEpoch) / 1000.0;
}
- if (resultStart >= 0 && eventStart >= 0 && resultStart >= eventStart) {
+ if (eventStart >= 0 && resultStart >= eventStart) {
Review comment:
same here
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/NexmarkLauncher.java
##########
@@ -302,7 +302,7 @@ private NexmarkPerf currentPerf(
effectiveEnd = eventEnd;
}
- if (effectiveEnd >= 0 && eventStart >= 0 && effectiveEnd >= eventStart) {
+ if (eventStart >= 0 && effectiveEnd >= eventStart) {
Review comment:
true that this expression is equivalent and simpler but less readable. I would prefer a revert
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Monitor.java
##########
@@ -39,20 +39,15 @@
private class MonitorDoFn extends DoFn<T, T> {
final Counter elementCounter = Metrics.counter(name, prefix + ".elements");
final Counter bytesCounter = Metrics.counter(name, prefix + ".bytes");
- final Distribution startTime = Metrics.distribution(name, prefix + ".startTime");
- final Distribution endTime = Metrics.distribution(name, prefix + ".endTime");
- final Distribution startTimestamp = Metrics.distribution(name, prefix + ".startTimestamp");
- final Distribution endTimestamp = Metrics.distribution(name, prefix + ".endTimestamp");
+ final Distribution systemTimestamp = Metrics.distribution(name, prefix + ".systemTimestamp");
+ final Distribution eventTimestamp = Metrics.distribution(name, prefix + ".eventTimestamp");
@ProcessElement
public void processElement(ProcessContext c) {
elementCounter.inc();
bytesCounter.inc(c.element().sizeInBytes());
- long now = System.currentTimeMillis();
- startTime.update(now);
- endTime.update(now);
- startTimestamp.update(c.timestamp().getMillis());
- endTimestamp.update(c.timestamp().getMillis());
+ systemTimestamp.update(System.currentTimeMillis());
Review comment:
agree with your simplification for these reasons:
- for each element processed, the distribution metrics start and end will be updated with the same value and, as the distribution reports min, max and average, and, as in _MetricsReader_ we are requesting the minimum and the maximum, there is no impact of having a single distribution
- the methods in _MetricsReader_ use a SplitIterator on the distributions that is not immutable for the underlying source (which could explain that there was 2 distributions created) but it does only reading (by calling _getMin()_ and _getMax()_) so there will be no side effect on the underlying distribution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-756656272
my bad, I merged but I forgot to tell you to rename the commit with ticket number and to squash them. It is not that serious, just not very clear in history. you'll know for next time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-756629398
Run Direct Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735716820
@rHermes Thanks! Could you create a Jira for this and squash all your commits into one?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes edited a comment on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes edited a comment on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-746806274
> Thanks for your work @rHermes !
> Only some minor fixes and an important comment.
> Nexmark execution times are similar to CI. So LGTM
> On piece of advice, it is your second cleaning PR on nexmark. Next time, could you please consider submitting cleaning PRs all in a single PR with separate commits with explicit messages? thanks
I thought it was better practice to create separate PRs, but it's easier for me to just combine them, so I'll do that next time :)
Sorry for not answering sooner @echauchot life caught up to me :sweat_smile:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev edited a comment on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev edited a comment on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737369304
@rHermes Thanks! Could you elaborate changes regarding this part `This PR ... merges the start and end distributions.` ? Why it was done?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737856613
@rHermes thanks for your work ! taking a look
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735793438
OK, squashed and created a JIRA issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot merged pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot merged pull request #13435:
URL: https://github.com/apache/beam/pull/13435
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev edited a comment on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev edited a comment on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737369304
@rHermes Thanks! Could you elaborate the changes regarding this part `This PR ... merges the start and end distributions.` ? Why it was done?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-756629582
Run Direct Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-739907528
> @rHermes thanks for your work ! taking a look
Any update on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735412366
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-747291549
@rHermes yes general good practice is to separate PRs that deal with different subjects. But for cleaning PRs, it is the same subject. There a lot of leftovers in nexmark what I would like to avoid is tens of PRs that remove only a couple of fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735435647
Seems I've been a bit quick in the trigger here and changed the logic after merging the two distributions. I'll push a fix later this evening, sorry for the slip!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on a change in pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on a change in pull request #13435:
URL: https://github.com/apache/beam/pull/13435#discussion_r553170443
##########
File path: sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Monitor.java
##########
@@ -39,20 +39,15 @@
private class MonitorDoFn extends DoFn<T, T> {
final Counter elementCounter = Metrics.counter(name, prefix + ".elements");
final Counter bytesCounter = Metrics.counter(name, prefix + ".bytes");
- final Distribution startTime = Metrics.distribution(name, prefix + ".startTime");
- final Distribution endTime = Metrics.distribution(name, prefix + ".endTime");
- final Distribution startTimestamp = Metrics.distribution(name, prefix + ".startTimestamp");
- final Distribution endTimestamp = Metrics.distribution(name, prefix + ".endTimestamp");
+ final Distribution systemTimestamp = Metrics.distribution(name, prefix + ".systemTimestamp");
Review comment:
please rename also the metric name
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes edited a comment on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes edited a comment on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735382003
The spotless check failed with a file I have not edited :confused: I can apply the spotless fix and upload that here if you want?
EDIT: I applied the spot checks when also merging the distributions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot merged pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot merged pull request #13435:
URL: https://github.com/apache/beam/pull/13435
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-746806274
> Thanks for your work @rHermes !
> Only some minor fixes and an important comment.
> Nexmark execution times are similar to CI. So LGTM
> On piece of advice, it is your second cleaning PR on nexmark. Next time, could you please consider submitting cleaning PRs all in a single PR with separate commits with explicit messages? thanks
I thought it was better practice to create separate PRs, but it's easier for me to just combine them, so I'll do that next time :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737513781
> @rHermes Thanks! Could you elaborate the changes regarding this part `This PR ... merges the start and end distributions.` ? Why it was done?
Because they are updated with exactly the same values, they will always be exactly the same, so they could be merged. If you look at the removed lines in Monitor you will see that they where set with exactly the same values
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-756140664
> @rHermes sorry for the delay. I took a llok at your changes. you just forgot a rename and I can merge.
Ahh, my bad. Fixed now :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737237673
@aromanenko-dev does it look good now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737370461
@echauchot Could you take a look on this PR since you worked on Nexmark too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735647662
I've fixed the error, but the error now is not in a part of the code that I haven't touched.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735680492
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rHermes commented on pull request #13435: Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
rHermes commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-735745797
> @rHermes Thanks! Could you create a Jira for this and squash all your commits into one?
Will do it after work today :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-741707048
@rHermes sorry I'm late on that I started the review and then I was dragged on other priorities. I think I could finish by the end of the week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-737369304
@rHermes Thanks! Could you elaborate changes regarding this part `This PR ... merges the start and end distributions.` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #13435: [BEAM-11358] Remove unused endOfStreamMonitor from nexmark and remove redundant distributions from monitor
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #13435:
URL: https://github.com/apache/beam/pull/13435#issuecomment-756629398
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org