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-2677] consider all sampled tuples which took greater than 0 ms processing time #2251

Closed
wants to merge 1 commit into from

Conversation

@vinodkc
Copy link
Contributor

commented Aug 1, 2017

Set delta to -1 , so that time delay for tuples that aren't sampled will not be added in BoltExecutorStats.
This is a followup PR of #2185 to fix in 1.x version

@vinodkc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

retest this

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

The failure looks unrelated.
+1

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

Btw it is clearly a bug, but in opposite to STORM-2613, because both nil and 0 are logically false. So it doesn't include tuples which are not sampled, but it also doesn't include tuples which are done in 0 ms. Less harmful but still affects the overall latency.

@vinodkc
I guess we need to file different issue for this, since the bug is affecting differently. Could you please follow up? Thanks in advance!

@vinodkc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

@HeartSaVioR
Sure, filed a new issue STORM-2677.
Can you please change the status of STORM-2613.

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2017

@vinodkc
OK. Please squash commits into one with changing commit title corresponding to STORM-2677. Please also change PR title as well.

@vinodkc vinodkc changed the title [STORM-2613] Tuples that aren't sampled shouldn't be considered for latency calculation [STORM-2613] Tuples that aren't sampled or took 0ms shouldn't be considered for latency calculation Aug 3, 2017

@vinodkc vinodkc force-pushed the vinodkc:br_fix_stat_issue_in_1.x branch from d90508b to 602c877 Aug 3, 2017

@vinodkc vinodkc changed the title [STORM-2613] Tuples that aren't sampled or took 0ms shouldn't be considered for latency calculation [STORM-2677] consider all sampled tuples which took greater than 0 ms processing time Aug 3, 2017

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2017

@vinodkc Thanks for changing the PR title. Could you change commit title as well?

@vinodkc vinodkc force-pushed the vinodkc:br_fix_stat_issue_in_1.x branch from 602c877 to e8e9efa Aug 4, 2017

@vinodkc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

@HeartSaVioR , Thanks for the review comments.
I've changed commit title

asfgit pushed a commit that referenced this pull request Aug 4, 2017
asfgit pushed a commit that referenced this pull request Aug 5, 2017
asfgit pushed a commit that referenced this pull request Aug 5, 2017
@HeartSaVioR

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2017

@vinodkc I merged this with some modification (commit title) but auto-close didn't work because it is pointing to non master branch. Could you close this? Thanks again for contribution!

@vinodkc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2017

Thanks.

@vinodkc vinodkc closed this Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.