STORM-2190: reduce contention between submission and scheduling #1764
Conversation
| @@ -90,7 +90,9 @@ public static void main(String[] args) throws Exception { | |||
| if (args != null && args.length > 0) { | |||
| conf.setNumWorkers(3); | |||
|
|
|||
| StormSubmitter.submitTopologyWithProgressBar(args[0], conf, builder.createTopology()); | |||
| for (String name: args) { | |||
There was a problem hiding this comment.
See comment on #1765. Is this change an artifact of manual testing?
|
One question about the for loop in WordCountTopology. I'm +1 once that's answered/addressed. |
| (into {}) | ||
| (.assignSlots inimbus topologies))) | ||
| (log-message "not a leader, skipping assignments"))) | ||
| (locking (:sched-lock nimbus) |
There was a problem hiding this comment.
Couldn't a wrong ordering of events happen since we are locking when calculating a scheduling then unlocking and then locking and uploading the new scheduling and unlocking
for example:
T0: submit
T1: rebalance
T2: rebalance - calculate new scheduling
T3: submit - calculate new scheduling
T4: rebalance - upload new scheduling to zk
T5: submit - upload new scheduling to zk
even though we should end up with the scheduling calculated by the rebalance but we end up with scheduling calculated from the original submit.
There was a problem hiding this comment.
@jerrypeng You are correct that this could happen. I don't really think it will be that likely to happen in practice but I'll think about it and see if we can fix it.
There was a problem hiding this comment.
maybe its also time to start thinking about decentralized scheduling mechanisms if certain scheduling strategies may take a while to compute a schedule, but that would require a major overhaul in nimbus.
There was a problem hiding this comment.
I agree and now that nimbus is in java we can look at doing some refactoring along those lines. If you feel that we need to do it now and that this is a blocker I can spend some time looking into how to do that better.
|
Just so we don't miss the comment from @jerrypeng
Yes, that is correct. We should do something here, and he suggested that perhaps as part of a refactor of Nimbus we should look at support for long running scheduling. In the short term I think I might make scheduling and writing to ZK atomic, but long term I think I will file a JIRA to look at better scheduling. |
|
I made a few changes to thing. I fixed the race condition and I addressed the review comments, but I also put in some optimizations to storm submitter. We were literally calling getClusterInfo 3+ times for each topology submission, and because the ultimate goal of STORM-2190 is to make it more scalable this helps a lot. There is still some lock contention, but it is much better then it was before. If things look good here I will backport the changes to my other pull request. |
|
+1 |
|
+1. Please apply the optimization to #1765 as well. |
|
LGTM +1 @revans2 thanks for making the optimizations |
|
@HeartSaVioR I plan on doing that. |
|
+1 again. |
No description provided.