You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by wendyshusband <gi...@git.apache.org> on 2017/06/17 05:53:57 UTC
[GitHub] storm pull request #2164: [STORM-2557]: A bug in DisruptorQueue causing seve...
GitHub user wendyshusband opened a pull request:
https://github.com/apache/storm/pull/2164
[STORM-2557]: A bug in DisruptorQueue causing severe underestimation of…
Recently, we are tuning the performance of our topology and deploying some theoretical performance models that heavily rely on the metrics of query arrival rates. We found a bug in `DisruptorQueue `that leads to severe underestimation to the queue arrival rates. After further investigation, we finally found that in the current implementation of `DisruptorQueue`, the arrival rates are actually measured as the number of batches of tuples rather than the actual number of tuples, resulting in significant underestimation of the arrival rates.
To be more specific, in` DisruptorQueue.publishDirectSingle()` and `DisruptorQueue.publishDirect()` functions, objects containing tuples are published to the buffer and the metrics are notified by calling `_metric.notifyArrivals(1)`. This works fine when the object is simply a wrapper of a single tuple. However, the object could also be an instance of `ArrayList<AddressedTuple>` or `HashMap<Integer, ArrayList<TaskMessage>>`. In such case, we should get the actual number of tuples in the object and notify the `_metrics `with the right value.
I added some code that determine the type of object to fix this issue at `DisruptorQueue.publishDirectSingle ()` and `DisruptorQueue.publishDirectSingle ()` functions.
Any comment or suggestion regarding to this pull request is welcomed.
Thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wendyshusband/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2164.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2164
----
commit 70d45b29c5fd7d67e4d841a2cb2c8504fb999e2c
Author: wendyshusband <tk...@126.com>
Date: 2017-06-17T05:40:17Z
STORM-2557: A bug in DisruptorQueue causing severe underestimation of queue arrival rates
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by wendyshusband <gi...@git.apache.org>.
Github user wendyshusband commented on the issue:
https://github.com/apache/storm/pull/2164
I apologize for the lack of timely reply. thank you very much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the issue:
https://github.com/apache/storm/pull/2164
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2164: [STORM-2557]: A bug in DisruptorQueue causing seve...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2164#discussion_r122565494
--- Diff: storm-client/src/jvm/org/apache/storm/utils/DisruptorQueue.java ---
@@ -502,15 +502,26 @@ private static Long getId() {
private void publishDirectSingle(Object obj, boolean block) throws InsufficientCapacityException {
long at;
+ long numberOfTuples;
if (block) {
at = _buffer.next();
} else {
at = _buffer.tryNext();
}
AtomicReference<Object> m = _buffer.get(at);
m.set(obj);
+ if (obj instanceof ArrayList) {
--- End diff --
Seems like the logic for counting number of tuples is duplicated so better to extract method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the issue:
https://github.com/apache/storm/pull/2164
@revans2 I quite agree with you. The current publish logic in disruptor queue is difficult to follow. It will be great if some one could refine the code in a separate JIRA, so that we can easily know what are storing in the queue.
@HeartSaVioR Given the feedback from revans2, could you merge this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2164
I just realized that because the `sojourn_time_ms` is calculated based off of the request rate, that this updated, and the population (not updated and still based off of slots and not tuples), that this made the sojurn time horribly inaccurate. I'll file a JIRA to address it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by wangli1426 <gi...@git.apache.org>.
Github user wangli1426 commented on the issue:
https://github.com/apache/storm/pull/2164
@HeartSaVioR @asfgit It seems that we forgot to add @wendyshusband to the contributor list.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2164
@wendyshusband
Thanks for the contribution. Looks good. Minor comment.
@revans2 I guess you would want to review this as well, since it's related to disruptor batching.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2164
@wangli1426 It would be great if you could also add me to the contributor list too LOL
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2164
The change as is looks OK to me. My biggest problem with it is that there is now tight coupling between the Disruptor queue and storm itself, although looking at the code that was true before.
Now that we are pure java for both publishers and consumers I really would like to see us add in some generics so it is more obvious what we are storing in the queue, but that can be in a follow on JIRA.
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2164: [STORM-2557]: A bug in DisruptorQueue causing seve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2164
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2164: [STORM-2557]: A bug in DisruptorQueue causing seve...
Posted by wendyshusband <gi...@git.apache.org>.
Github user wendyshusband commented on a diff in the pull request:
https://github.com/apache/storm/pull/2164#discussion_r122567474
--- Diff: storm-client/src/jvm/org/apache/storm/utils/DisruptorQueue.java ---
@@ -502,15 +502,26 @@ private static Long getId() {
private void publishDirectSingle(Object obj, boolean block) throws InsufficientCapacityException {
long at;
+ long numberOfTuples;
if (block) {
at = _buffer.next();
} else {
at = _buffer.tryNext();
}
AtomicReference<Object> m = _buffer.get(at);
m.set(obj);
+ if (obj instanceof ArrayList) {
--- End diff --
Thank you for your suggestion. I have modified accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2164: [STORM-2557]: A bug in DisruptorQueue causing severe unde...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2164
OK. +1 from me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---