Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2754#discussion_r209311534
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1984,11 +2052,12 @@ private int fragmentedCpu() {
Cluster cluster = new Cluster(inimbus, supervisors,
topoToSchedAssignment, topologies, conf);
cluster.setStatusMap(idToSchedStatus.get());
- long beforeSchedule = System.currentTimeMillis();
+ schedulingStartTimeNs.set(Time.nanoTime());
scheduler.schedule(topologies, cluster);
- long scheduleTimeElapsedMs = System.currentTimeMillis() -
beforeSchedule;
- LOG.debug("Scheduling took {} ms for {} topologies",
scheduleTimeElapsedMs, topologies.getTopologies().size());
- scheduleTopologyTimeMs.update(scheduleTimeElapsedMs);
+ long elapsed = -schedulingStartTimeNs.getAndSet(null) +
Time.nanoTime();
--- End diff --
I might have asked this over on the other PR, but is it written this way to
fix the race? If so, please put a note in here about it, otherwise it's very
likely this gets reordered next time someone happens across this code.
Also splitting this line in two would probably also be appropriate.
```
//Get and set the start time before getting current time in order to avoid
potential race with the longestSchedulingTime gauge
long startTimeNs = schedulingStartTimeNs.getAndSet(null);
long nowNs = Time.nanoTime();
long elapsed = nowNs - startTimeNs;
```
It's much more obvious that the order matters this way.
---
