Skip to content
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

[STORM-3018] Fix integration test DemoTest#testExclamationTopology fa… #2619

Merged
merged 1 commit into from Apr 2, 2018

Conversation

danny0405
Copy link

@danny0405 danny0405 commented Apr 1, 2018

For STORM-2693 i changed the task metrics reporting interval default to 60 seconds, which is same with the DemoTest#testExclamationTopology check time.

So fix it here.

See STORM-3018

@danny0405 danny0405 force-pushed the fix-test branch 2 times, most recently from 4d6cff5 to 6e2001b Compare April 1, 2018 08:38
Copy link
Contributor

@srdo srdo left a comment

Choose a reason for hiding this comment

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

+1

@@ -195,7 +195,7 @@ worker.log.level.reset.poll.secs: 30
# control how many worker receiver threads we need per worker
topology.worker.receiver.thread.count: 1

# Executor metrics reporting interval.
# Executor metrics reporting interval, keep default val same with topology.builtin.metrics.bucket.size.secs
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could you add to the comment the reason why these must be kept in sync?

Copy link
Author

@danny0405 danny0405 Apr 1, 2018

Choose a reason for hiding this comment

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

Because:

  1. the internal metrics interval is set up by conf: topology.builtin.metrics.bucket.size.secs, we should keep sync with it
  2. Now we use ZK to store our metrics and the ui only show built in metrics, the metrics consumer's collecting interval are also default to 60 seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, but I meant add a note to the comment. I can't tell from the current comment why the two properties need to be in sync, so I'd have to look up this PR to see why.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, got your idea.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1

@asfgit asfgit merged commit 34c1857 into apache:master Apr 2, 2018
@danny0405 danny0405 deleted the fix-test branch April 2, 2018 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants