[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r314825951
##
File path:
storm-server/src/main/java/org/apache/storm/scheduler/IsolationScheduler.java
##
@@ -218,8 +218,8 @@ public void schedule(Topologies topologies, Cluster
cluster) {
List allExecutors = new ArrayList();
Collection> values = compExecutors.values();
-for (List eList : values) {
-allExecutors.addAll(eList);
+for (List value : values) {
+allExecutors.addAll(value);
Review comment:
OK, and it did not start with the minimum 2 lower-case letters. Thanks!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r314824929
##
File path:
storm-server/src/main/java/org/apache/storm/daemon/drpc/RequestFactory.java
##
@@ -21,5 +21,6 @@
import org.apache.storm.generated.DRPCRequest;
public interface RequestFactory {
-public T mkRequest(String function, DRPCRequest req);
+
+T mkRequest(String function, DRPCRequest req);
Review comment:
OK the first time I missed that `public` was removed as redundant. I had
thought only a new line was added.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r314824598
##
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##
@@ -462,6 +466,24 @@ public LocalTopology submitTopology(String topologyName,
Map con
return new LocalTopology(topologyName, topology);
}
+@Override
+public LocalTopology submitTopology(String topologyName, Map conf, TrackedTopology topology)
+throws TException {
+return submitTopology(topologyName, conf, topology.getTopology());
+}
+
+@Override
+public void submitTopology(String name, String uploadedJarLocation, String
jsonConf, StormTopology topology)
+throws AlreadyAliveException, InvalidTopologyException,
AuthorizationException, TException {
+try {
+@SuppressWarnings("unchecked")
+Map conf = (Map)
JSONValue.parseWithException(jsonConf);
+submitTopology(name, conf, topology);
+} catch (ParseException e) {
+throw new RuntimeException(e);
+}
+}
+
Review comment:
Ah now I understand. This seems good to me.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312710905
##
File path:
storm-server/src/main/java/org/apache/storm/nimbus/AssignmentDistributionService.java
##
@@ -186,7 +186,6 @@ private Integer nextQueueId() {
* Get an assignments from the target queue with the specific index.
* @param queueIndex index of the queue
* @return an {@link NodeAssignments}
- * @throws InterruptedException
Review comment:
Answered
[elsewhere](https://github.com/apache/storm/pull/3050#discussion_r312710620)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings URL: https://github.com/apache/storm/pull/3050#discussion_r312710850 ## File path: storm-server/src/main/java/org/apache/storm/pacemaker/PacemakerServer.java ## @@ -90,6 +90,7 @@ public PacemakerServer(IServerMessageHandler handler, Map config this.bossEventLoopGroup = new NioEventLoopGroup(1, bossFactory); // 0 means DEFAULT_EVENT_LOOP_THREADS // https://github.com/netty/netty/blob/netty-4.1.24.Final/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java#L40 +int maxWorkers = (int) config.get(DaemonConfig.PACEMAKER_MAX_THREADS); Review comment: I apologize, @krichter722 , it looked to me like the comment was resolved without a change, so I was confused by the diff. The code as it is currently looks fine to me. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312709531
##
File path: storm-server/src/main/java/org/apache/storm/Testing.java
##
@@ -314,6 +308,21 @@ public static void withTrackedCluster(MkClusterParam
param, TestJob code) {
}
}
+/**
+ * In a tracked topology some metrics are tracked. This provides a way to
get those metrics.
+ * This is intended mostly for internal testing.
+ *
+ * @param id the id of the tracked cluster
+ * @param key the name of the metric to get.
+ * @return the metric
+ */
+@SuppressWarnings("unchecked")
+@Deprecated
+public static int globalAmt(String id, String key) {
+LOG.warn("Reading tracked metrics for ID {}", id);
+return ((ConcurrentHashMap)
RegisteredGlobalState.getState(id)).get(key).get();
+}
+
Review comment:
Why was this moved?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312710089
##
File path:
storm-server/src/main/java/org/apache/storm/scheduler/IsolationScheduler.java
##
@@ -218,8 +218,8 @@ public void schedule(Topologies topologies, Cluster
cluster) {
List allExecutors = new ArrayList();
Collection> values = compExecutors.values();
-for (List eList : values) {
-allExecutors.addAll(eList);
+for (List value : values) {
+allExecutors.addAll(value);
Review comment:
What was the violation here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312709830
##
File path:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##
@@ -1277,6 +1277,132 @@ private static void validatePortAvailable(Map conf) throws IOExc
}
}
+@VisibleForTesting
+public void launchServer() throws Exception {
+try {
+IStormClusterState state = stormClusterState;
+NimbusInfo hpi = nimbusHostPortInfo;
+
+LOG.info("Starting Nimbus with conf {}",
ConfigUtils.maskPasswords(conf));
+validator.prepare(conf);
+
+//add to nimbuses
+state.addNimbusHost(hpi.getHost(),
+new NimbusSummary(hpi.getHost(), hpi.getPort(),
Time.currentTimeSecs(), false, STORM_VERSION));
+leaderElector.addToLeaderLockQueue();
+this.blobStore.startSyncBlobs();
+
+for (ClusterMetricsConsumerExecutor exec:
clusterConsumerExceutors) {
+exec.prepare();
+}
+
+if (isLeader()) {
+for (String topoId : state.activeStorms()) {
+transition(topoId, TopologyActions.STARTUP, null);
+}
+clusterMetricSet.setActive(true);
+}
+
+final boolean doNotReassign = (Boolean)
conf.getOrDefault(ServerConfigUtils.NIMBUS_DO_NOT_REASSIGN, false);
+timer.scheduleRecurring(0,
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_MONITOR_FREQ_SECS)),
+() -> {
+try {
+if (!doNotReassign) {
+mkAssignments();
+}
+doCleanup();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+
+// Schedule Nimbus inbox cleaner
+final int jarExpSecs =
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_INBOX_JAR_EXPIRATION_SECS));
+timer.scheduleRecurring(0,
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_CLEANUP_INBOX_FREQ_SECS)),
+() -> {
+try {
+cleanInbox(getInbox(), jarExpSecs);
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+
+
+// Schedule topology history cleaner
+Integer interval =
ObjectReader.getInt(conf.get(DaemonConfig.LOGVIEWER_CLEANUP_INTERVAL_SECS),
null);
+if (interval != null) {
+final int lvCleanupAgeMins =
ObjectReader.getInt(conf.get(DaemonConfig.LOGVIEWER_CLEANUP_AGE_MINS));
+timer.scheduleRecurring(0, interval,
+() -> {
+try {
+cleanTopologyHistory(lvCleanupAgeMins);
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
+
+timer.scheduleRecurring(0,
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_CREDENTIAL_RENEW_FREQ_SECS)),
+() -> {
+try {
+renewCredentials();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+
+
metricsRegistry.registerGauge("nimbus:total-available-memory-non-negative", ()
-> nodeIdToResources.get().values()
+.parallelStream()
+.mapToDouble(supervisorResources ->
Math.max(supervisorResources.getAvailableMem(), 0))
+.sum());
+metricsRegistry.registerGauge("nimbus:available-cpu-non-negative",
() -> nodeIdToResources.get().values()
+.parallelStream()
+.mapToDouble(supervisorResources ->
Math.max(supervisorResources.getAvailableCpu(), 0))
+.sum());
+metricsRegistry.registerGauge("nimbus:total-memory", () ->
nodeIdToResources.get().values()
+.parallelStream()
+.mapToDouble(SupervisorResources::getTotalMem)
+.sum());
+metricsRegistry.registerGauge("nimbus:total-cpu", () ->
nodeIdToResources.get().values()
+.parallelStream()
+.mapToDouble(SupervisorResources::getTotalCpu)
+.sum());
+metricsRegistry.registerGauge("nimbus:longest-scheduling-time-ms",
() -> {
+//We want to update longest scheduling time in real time in
case scheduler get stuck
+// Get current time before startTime to avoid potential race
with scheduler's Timer
+Long currTime =
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312709596
##
File path:
storm-server/src/main/java/org/apache/storm/daemon/drpc/RequestFactory.java
##
@@ -21,5 +21,6 @@
import org.apache.storm.generated.DRPCRequest;
public interface RequestFactory {
-public T mkRequest(String function, DRPCRequest req);
+
+T mkRequest(String function, DRPCRequest req);
Review comment:
Is there a checkstyle policy that requires changes like these?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312710003
##
File path:
storm-server/src/main/java/org/apache/storm/nimbus/AssignmentDistributionService.java
##
@@ -186,7 +186,6 @@ private Integer nextQueueId() {
* Get an assignments from the target queue with the specific index.
* @param queueIndex index of the queue
* @return an {@link NodeAssignments}
- * @throws InterruptedException
Review comment:
Why was this removed?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings URL: https://github.com/apache/storm/pull/3050#discussion_r312710316 ## File path: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ## @@ -214,13 +219,6 @@ public static ClientBlobStore getClientBlobStoreForSupervisor(Map
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312709484
##
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##
@@ -462,6 +466,24 @@ public LocalTopology submitTopology(String topologyName,
Map con
return new LocalTopology(topologyName, topology);
}
+@Override
+public LocalTopology submitTopology(String topologyName, Map conf, TrackedTopology topology)
+throws TException {
+return submitTopology(topologyName, conf, topology.getTopology());
+}
+
+@Override
+public void submitTopology(String name, String uploadedJarLocation, String
jsonConf, StormTopology topology)
+throws AlreadyAliveException, InvalidTopologyException,
AuthorizationException, TException {
+try {
+@SuppressWarnings("unchecked")
+Map conf = (Map)
JSONValue.parseWithException(jsonConf);
+submitTopology(name, conf, topology);
+} catch (ParseException e) {
+throw new RuntimeException(e);
+}
+}
+
Review comment:
We've moved some code around here, but I also see we have suppressed a
checkstyle warning. Are both of these changes necessary?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312710262
##
File path:
storm-server/src/main/java/org/apache/storm/scheduler/multitenant/Node.java
##
@@ -130,95 +133,110 @@ public static int countTotalSlotsAlive(Collection
nodes) {
}
public String getId() {
-return _nodeId;
+return nodeId;
}
public boolean isAlive() {
-return _isAlive;
+return isAlive;
}
/**
+ * Get running topologies.
* @return a collection of the topology ids currently running on this node
*/
public Collection getRunningTopologies() {
-return _topIdToUsedSlots.keySet();
+return topIdToUsedSlots.keySet();
}
public boolean isTotallyFree() {
-return _topIdToUsedSlots.isEmpty();
+return topIdToUsedSlots.isEmpty();
}
public int totalSlotsFree() {
-return _freeSlots.size();
+return freeSlots.size();
}
public int totalSlotsUsed() {
int total = 0;
-for (Set slots : _topIdToUsedSlots.values()) {
+for (Set slots : topIdToUsedSlots.values()) {
total += slots.size();
}
return total;
}
-public int totalSlots() {
-return totalSlotsFree() + totalSlotsUsed();
-}
-
public int totalSlotsUsed(String topId) {
int total = 0;
-Set slots = _topIdToUsedSlots.get(topId);
+Set slots = topIdToUsedSlots.get(topId);
if (slots != null) {
total = slots.size();
}
return total;
}
+public int totalSlots() {
+return totalSlotsFree() + totalSlotsUsed();
+}
+
private void validateSlot(WorkerSlot ws) {
-if (!_nodeId.equals(ws.getNodeId())) {
-throw new IllegalArgumentException(
-"Trying to add a slot to the wrong node " + ws +
-" is not a part of " + _nodeId);
+if (!nodeId.equals(ws.getNodeId())) {
+throw new IllegalArgumentException("Trying to add a slot to the
wrong node "
++ ws
++ " is not a part of "
++ nodeId);
}
}
private void addOrphanedSlot(WorkerSlot ws) {
-if (_isAlive) {
-throw new IllegalArgumentException("Orphaned Slots " +
- "only are allowed on dead
nodes.");
+if (isAlive) {
+throw new IllegalArgumentException("Orphaned Slots only are
allowed on dead nodes.");
}
validateSlot(ws);
-if (_freeSlots.contains(ws)) {
+if (freeSlots.contains(ws)) {
return;
}
-for (Set used : _topIdToUsedSlots.values()) {
+for (Set used : topIdToUsedSlots.values()) {
if (used.contains(ws)) {
return;
}
}
-_freeSlots.add(ws);
+freeSlots.add(ws);
}
boolean assignInternal(WorkerSlot ws, String topId, boolean dontThrow) {
validateSlot(ws);
-if (!_freeSlots.remove(ws)) {
-for (Entry> topologySetEntry :
_topIdToUsedSlots.entrySet()) {
+if (!freeSlots.remove(ws)) {
+for (Entry> topologySetEntry :
topIdToUsedSlots.entrySet()) {
if (topologySetEntry.getValue().contains(ws)) {
if (dontThrow) {
-LOG.warn("Worker slot [" + ws + "] can't be assigned
to " + topId +
- ". Its already assigned to " +
topologySetEntry.getKey() + ".");
+LOG.warn("Worker slot ["
++ ws
++ "] can't be assigned to "
++ topId
++ ". Its already assigned to "
++ topologySetEntry.getKey()
++ ".");
Review comment:
This kind of enforced alignment will further encourage the use of format
strings rather than string concatenation, because it is much less readable. I
think this is a good thing.
I am not suggesting we change all such occurrences to use format strings
now—if we see a performance issue or if we want to change it for readability
later I consider it out of scope for this pull request.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings URL: https://github.com/apache/storm/pull/3050#discussion_r312710065 ## File path: storm-server/src/main/java/org/apache/storm/pacemaker/PacemakerServer.java ## @@ -90,6 +90,7 @@ public PacemakerServer(IServerMessageHandler handler, Map config this.bossEventLoopGroup = new NioEventLoopGroup(1, bossFactory); // 0 means DEFAULT_EVENT_LOOP_THREADS // https://github.com/netty/netty/blob/netty-4.1.24.Final/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java#L40 +int maxWorkers = (int) config.get(DaemonConfig.PACEMAKER_MAX_THREADS); Review comment: I agree with @srdo on this one. If moving this up one line would cause a style violation, how about moving it down one line? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [storm] d2r commented on a change in pull request #3050: STORM-3434: server: fix all checkstyle warnings
d2r commented on a change in pull request #3050: STORM-3434: server: fix all
checkstyle warnings
URL: https://github.com/apache/storm/pull/3050#discussion_r312709977
##
File path:
storm-server/src/main/java/org/apache/storm/localizer/LocalizedResourceRetentionSet.java
##
@@ -134,6 +134,7 @@ public String toString() {
return "Cache: " + currentSize;
}
+@SuppressWarnings("checkstyle:AbbreviationAsWordInName")
Review comment:
I appreciate the judicious use of these suppressions.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
