[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
GitHub user kishorvpatil opened a pull request: https://github.com/apache/storm/pull/2603 [STORM-3003] Adding Assignment caching to Nimbus Since nimbus ( scheduler generates assignments) it can cache it instead of polling for it from ZK or other state manager. This would improve scheduling iteration time, as well as all UI pages that require assignment information. The need for this improvement felt when we noticed this is larger clusters where ZK continues to be bottleneck. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kishorvpatil/incubator-storm storm3003 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2603.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 #2603 commit c0e460ccc74872a7b716e72445f0a10247b6f450 Author: Kishor Patil Date: 2018-03-22T20:43:00Z Adding Assignment caching to Nimbus ---
[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2603#discussion_r176777374
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1944,6 +1957,7 @@ private void mkAssignments(String scratchTopoId)
throws Exception {
} else {
LOG.info("Setting new assignment for topology id {}:
{}", topoId, assignment);
state.setAssignment(topoId, assignment);
+assignmentsCache.get().put(topoId, assignment);
--- End diff --
Could we remove the dead code comment in this `for` block?
---
[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2603#discussion_r176786400
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1754,7 +1762,7 @@ private TopologyResources
getResourcesForTopology(String topoId, StormBase base)
try {
IStormClusterState state = stormClusterState;
TopologyDetails details = readTopologyDetails(topoId,
base);
-Assignment assignment = state.assignmentInfo(topoId, null);
+Assignment assignment = getAssignmentInfo(state, topoId);
--- End diff --
We can just use `stormClusterState` directly without making another `state`
variable. (Here and in the next method.)
---
[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2603#discussion_r176771823
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1196,6 +1198,12 @@ private IStormClusterState getStormClusterState() {
return heartbeatsCache;
}
+@VisibleForTesting
+public AtomicReference> getAssignmentsCache() {
+return assignmentsCache;
+}
+
+
--- End diff --
* What uses this? Should we remove it?
* In fact, same question for `getHeartbeatsCache`
* nitpick: Else, can we remove the extra line here?
---
[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2603#discussion_r177179410
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1944,6 +1957,7 @@ private void mkAssignments(String scratchTopoId)
throws Exception {
} else {
LOG.info("Setting new assignment for topology id {}:
{}", topoId, assignment);
state.setAssignment(topoId, assignment);
+assignmentsCache.get().put(topoId, assignment);
--- End diff --
This was addressed.
---
[GitHub] storm pull request #2603: [STORM-3003] Adding Assignment caching to Nimbus
Github user kishorvpatil closed the pull request at: https://github.com/apache/storm/pull/2603 ---
