Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/521
---
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 enabl
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-106087952
I'm +1. As I mentioned on the other pull request this patch actually
improves performance while the alternate approach created a performance
regression.
---
If your pro
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-106087173
@ptgoetz Could you take a look in order to complete current PR? Thanks in
advance!
---
If your project is set up for it, you can reply to this email and have your
re
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-106086856
@d2r No Problem, thanks for reviewing and taking performance test!
---
If your project is set up for it, you can reply to this email and have your
reply appear on Git
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-106038982
Tested with https://github.com/yahoo/storm-perf-test with the following
arguments:
```
--ack --bolt 4 --name test -l 1 -n 1 --workers 4 --spout 3 --testTimeSec 900
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-104045430
@d2r @ptgoetz Applied @d2r's comments.
---
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
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30750154
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30750199
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103992667
@HeartSaVioR,
#521 passed my fault tolerance test (which randomly kills workers and tests
for data loss).
I'd suggest closing this pull request, and reopening
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103928314
Testing with #521 applied to 0.10.x-branch I'm actually seeing a
performance ***improvement***.
With core storm topologies there's an increase in throughput and a
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103904248
I'm definitely seeing a performance regression. In a core storm topology
I'm seeing a drop in throughput and slight increase in latency. In a trident
topology I'm seeing
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103843197
Since current approach do more works within read lock, it may be necessary
to test performance with previous approach, too.
---
If your project is set up for it, you
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103777141
I'm also seeing a performance regression. IMO resolving issue could
introduce performance drop slightly, but shouldn't too much.
---
If your project is set up for it
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103668907
So far I think I'm seeing a performance regression in terms of throughput,
but I want to let all the test cases run to be sure. I will have more
information tomorrow.
-
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103575069
FYI: unit tests passed for me.
I plan to take a look at performance using
[storm-perf-test](https://github.com/yahoo/storm-perf-test)
---
If your project is set up for i
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/557#issuecomment-103230182
@d2r Changed to have a buffer outer side of TransferDrainer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/557#discussion_r30549645
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,88 +26,47 @@
public class TransferDrainer {
- private HashMa
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/557#discussion_r30549032
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,88 +26,47 @@
public class TransferDrainer {
- privat
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/557#discussion_r30548714
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,88 +26,47 @@
public class TransferDrainer {
- private HashMa
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-103213278
@d2r Thanks for reviewing!
I'd like to get 6ef2f11 to pulled in, so I'm closing this PR unless we
agree #521 is better than #557.
---
If your project is set up f
Github user HeartSaVioR closed the pull request at:
https://github.com/apache/storm/pull/521
---
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
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/557
[STORM-737] Check task->node+port with read lock to prevent sending to
closed connection
* we can ensure task->node+port is safe within read lock
** refer write lock inside of mk-refresh-conn
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-103187844
> I made another changeset which reverts whole things to 0666c41 (without
try-serialize-local) but leaves TransferDrainer as buffer and grouper (by
host+port).
> HeartSaV
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30539714
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -129,12 +129,12 @@
(.add local pair)
;;Usi
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30539645
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
import b
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30539591
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
import b
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-103145088
@HeartSaVioR,
I see you are right. There are two race conditions here:
1. If we do not check assignments in the read-lock, then we could use an
invalid conn
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30521681
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
;;Usi
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30513365
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30511279
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
;;Usi
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-102749917
Nathan's comments for clarifying its real issue completely makes sense.
But we should verify what codes we should change while applying.
IMO it is a critical pa
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30466962
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30466949
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30463289
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -23,40 +23,62 @@
import backtype.storm.messaging.IConnection;
import b
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30463274
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
;;Usi
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101841901
I made another changeset which reverts whole things to 0666c41 (without
```try-serialize-local```) but leaves TransferDrainer as buffer and grouper (by
host+port).
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101831525
We may be better to revert mk-transfer-fn to
0666c41387fc11c0422b26ab27ebc38c30fe26af as grouping by task can be handled (or
doesn't need to be handled) from mk-trans
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101826046
@clockfly
I'd like you to have a look since I don't want to destroy your
optimization, but just fix the issue.
Thanks in advance!
---
If your project is set
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101825293
@d2r
Modified TransferDrainer to re-group messages by destination when send has
called.
I left TransferDrainer.add() cause it plays as buffer, but I'm ready t
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101648482
> @d2r I can't reproduce test failures. I'll give it a try again.
OK, please update your branch, and I will re-run the tests.
---
If your project is set up for it, y
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30225056
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
;;Usi
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101515533
@d2r I can't reproduce test failures. I'll give it a try again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30201415
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,10 +26,10 @@
public class TransferDrainer {
- privat
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30198050
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30197889
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30197344
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,10 +26,10 @@
public class TransferDrainer {
- privat
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30197303
--- Diff: storm-core/src/clj/backtype/storm/daemon/worker.clj ---
@@ -139,12 +139,12 @@
(.add local pair)
;;Usi
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101480982
I see 4 test errors using your branch. They all appear to be test timeouts.
* integration-test/test-basic-topology
* messaging-test/
* test-receiver-messag
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/521#discussion_r30196125
--- Diff: storm-core/src/jvm/backtype/storm/utils/TransferDrainer.java ---
@@ -26,10 +26,10 @@
public class TransferDrainer {
- private HashMa
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101475237
> @d2r
> First of all, I'm sorry I did some mistakes about expression.
No worries.
> So, I'd like you to review current PR, and find out issues, and go
t
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101449371
Please note that packets shouldn't have values which can be changed while
passing to transfer flow.
task->node+port can be changed (I mean node+port for tuple can
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101446038
@d2r
First of all, I'm sorry I did some mistakes about expression.
I mean, your approach follows some of Nathan's comment, but not all.
Your patch can also
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-101382574
@HeartSaVioR
Thanks for taking a look. Well, I was trying to follow his comments. :)
I noticed though, that the line in my `let` binding
```Clojure
v
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-98942013
@d2r Thanks for comment, I can wait it. :)
Btw, your branch doesn't follow Nathan's comment.
```
In short, the code in the write-lock is fine â it's t
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-98857155
@HeartSaVioR , I am sorry I have been swamped with another task, and I have
not had a chance to review.
I had a
[branch](https://github.com/apache/storm/compare/maste
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/521#issuecomment-98850849
@nathanmarz @d2r You may want to take a look since it's based on your
discussion, https://github.com/apache/storm/pull/349#issuecomment-87778672.
---
If your project
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/521
[STORM-737] Check task->node+port with read lock to prevent sending to
closed connection
It's based on Nathan's comment, please refer to
https://github.com/apache/storm/pull/349#issuecomment-877
57 matches
Mail list logo
