[GitHub] storm pull request #2803: STORM-2578: Apply new code style to storm-elastics...
Github user milantracy commented on a diff in the pull request:
https://github.com/apache/storm/pull/2803#discussion_r210499451
--- Diff:
external/storm-elasticsearch/src/main/java/org/apache/storm/elasticsearch/bolt/EsLookupBolt.java
---
@@ -76,12 +78,12 @@ public void process(Tuple tuple) {
}
private Collection lookupValuesInEs(Tuple tuple) throws
IOException {
- String index = tupleMapper.getIndex(tuple);
- String type = tupleMapper.getType(tuple);
- String id = tupleMapper.getId(tuple);
- Map params = tupleMapper.getParams(tuple, new
HashMap<>());
+String index = tupleMapper.getIndex(tuple);
+String type = tupleMapper.getType(tuple);
+String id = tupleMapper.getId(tuple);
+Map params = tupleMapper.getParams(tuple, new
HashMap<>());
--- End diff --
hi Danny, we might be not allowed to indent with tab because of
FileTabCharacter in checkstyle. Please correct me if I misunderstood here.
---
[GitHub] storm issue #2803: STORM-2578: Apply new code style to storm-elasticsearch
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2803 @milantracy Thanks for the contribution! Could you please reduce the number of max violation count so that we can see how many spots your patch address, and also we never break it again? ---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2805#discussion_r210684974 --- Diff: pom.xml --- @@ -280,7 +280,7 @@ 1.6.6 2.8.2 1.7.21 -3.1.0 +3.2.6 --- End diff -- Yes, sorry. The previous version didn't have the methods from this PR https://github.com/dropwizard/metrics/pull/1023/files. We need them so we can get `getOrAdd` behavior (either add the metric if not registered, or return the existing metric) for metric registration that uses factory functions. Without this ability, registering metrics (especially gauges) would be a pain, since we'd have to be very careful only to register them once. ---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Github user agresch commented on a diff in the pull request: https://github.com/apache/storm/pull/2805#discussion_r210680050 --- Diff: pom.xml --- @@ -280,7 +280,7 @@ 1.6.6 2.8.2 1.7.21 -3.1.0 +3.2.6 --- End diff -- can you comment on the version change? ---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210681943
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java
---
@@ -622,6 +620,12 @@ protected String javaCmd(String cmd) {
List classPathParams = getClassPathParams(stormRoot,
topoVersion);
List commonParams = getCommonParams();
+String log4jConfigurationFile = getWorkerLoggingConfigFile();
--- End diff --
Nit: Can this go in `getCommonParams`?
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2805#discussion_r210685351 --- Diff: pom.xml --- @@ -280,7 +280,7 @@ 1.6.6 2.8.2 1.7.21 -3.1.0 +3.2.6 --- End diff -- This specific version just happens to be the latest in the 3.x release line. I'd like to upgrade to the latest version of the library, but we'll have to pull out a bit of code relating to the Ganglia reporter first, and I didn't want to mix it in to this PR. https://github.com/dropwizard/metrics/issues/1319 ---
[GitHub] storm pull request #2800: STORM-3162: Fix concurrent modification bug
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2800#discussion_r210633216
--- Diff: storm-client/src/jvm/org/apache/storm/stats/StatsUtil.java ---
@@ -1565,23 +1565,26 @@ public static ComponentPageInfo aggCompExecsStats(
public static void updateHeartbeatCache(Map, Map> cache,
Map, Map> executorBeats, Set> executors,
Integer timeout) {
-//if not executor beats, refresh is-timed-out of the cache which
is done by master
+assert cache instanceof ConcurrentMap;
+//Should we enforce update-if-newer policy?
if (executorBeats == null) {
-for (Map.Entry, Map>
executorbeat : cache.entrySet()) {
-Map beat = executorbeat.getValue();
+//If not executor beats, refresh is-timed-out of the cache
which is done by master
+for (Map.Entry, Map>
executorBeat : cache.entrySet()) {
+Map beat = executorBeat.getValue();
beat.put("is-timed-out", Time.deltaSecs((Integer)
beat.get("nimbus-time")) >= timeout);
}
-return;
-}
-//else refresh nimbus-time and executor-reported-time by
heartbeats reporting
-for (List executor : executors) {
-cache.put(executor, updateExecutorCache(cache.get(executor),
executorBeats.get(executor), timeout));
+} else {
--- End diff --
Nit: Explicit return can help decrease indentation, I liked it better
before.
---
[GitHub] storm pull request #2800: STORM-3162: Fix concurrent modification bug
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2800#discussion_r210632176
--- Diff: storm-client/src/jvm/org/apache/storm/stats/StatsUtil.java ---
@@ -1525,27 +1528,24 @@ public static ComponentPageInfo aggCompExecsStats(
* @param timeout timeout
* @return a HashMap of updated executor heart beats
*/
-public static Map, Map>
updateHeartbeatCacheFromZkHeartbeat(Map, Map>
cache,
-
Map, Map>
-
executorBeats,
-
Set> executors,
-
Integer timeout) {
-Map, Map> ret = new HashMap<>();
-if (cache == null && executorBeats == null) {
-return ret;
-}
-
+public static ConcurrentMap, Map>
updateHeartbeatCacheFromZkHeartbeat(Map, Map>
cache,
+
Map, Map>
+
executorBeats,
+
Set> executors,
+
Integer timeout) {
if (cache == null) {
+if (executorBeats == null) {
--- End diff --
The construction here seems a little odd. Initializing cache and
executorBeats when null isn't necessary. I think we should keep the "if cache
and executorBeats are null" clause, then replace the other two branches with
something like `Map currBeat = cache == null ? null :
cache.get(executor);` and equivalent for `newBeat`.
---
[GitHub] storm issue #2800: STORM-3162: Fix concurrent modification bug
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2800 Fixed the nimbus_test https://github.com/zd-project/storm/pull/1 ---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2806#discussion_r210683327 --- Diff: pom.xml --- @@ -278,7 +278,7 @@ 4.1.25.Final 1.0.2 1.6.6 -2.8.2 +2.11.0 --- End diff -- Unless we know of a reason not to, we could just bump to the latest version IMO. ---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210682266
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -705,6 +705,13 @@
*/
@isString(acceptedValues = { "S0", "S1", "S2", "S3" })
public static final String TOPOLOGY_LOGGING_SENSITIVITY =
"topology.logging.sensitivity";
+/**
+ * Log file the user can use to configure Log4j2.
--- End diff --
Nit: Consider mentioning that the configuration file is applied in addition
to the regular config file, using these rules
https://logging.apache.org/log4j/2.x/manual/configuration.html#CompositeConfiguration.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Github user agresch commented on a diff in the pull request: https://github.com/apache/storm/pull/2805#discussion_r210690260 --- Diff: pom.xml --- @@ -280,7 +280,7 @@ 1.6.6 2.8.2 1.7.21 -3.1.0 +3.2.6 --- End diff -- Thanks. Just like to have a trail for version changes and the requirements. ---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
GitHub user srdo opened a pull request: https://github.com/apache/storm/pull/2805 STORM-3197: Make StormMetricsRegistry non-static https://issues.apache.org/jira/browse/STORM-3197 This also fixes https://issues.apache.org/jira/browse/STORM-3101 and https://issues.apache.org/jira/browse/STORM-3173. We talked about doing this in https://github.com/apache/storm/pull/2783 You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/storm STORM-3197 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2805.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 #2805 commit 3044239a841c96472eaedbb645e9fb29a32dcfb4 Author: Stig Rohde Døssing Date: 2018-07-30T20:53:08Z STORM-3197: Make StormMetricsRegistry non-static ---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
GitHub user jacobtolar opened a pull request: https://github.com/apache/storm/pull/2806 [STORM-3198] Topology submitters should be able to supply log4j2 conf This adds a new config setting, `topology.logging.config`, that allows a topology submitter to specify an additional log4j2 config to be used via composite configuration. When the worker starts the `-Dlog4j.configurationFile` option is modified to include the additional file if requested. To test, I submitted a topology (FastWordCountTopology): ``` bin/storm jar -c topology.logging.config=classpath:resources/topology_log4j2.xml ../../../../../examples/storm-starter/target/storm-starter-*.jar org.apache.storm.starter.FastWordCountTopology fwc ``` topology_log4j2.xml: ``` ``` With this child configuration added, the messages about [preparing bolts](https://github.com/apache/storm/blob/26d2f955251c0fa56520f6fe08cb35ef5171f321/storm-client/src/jvm/org/apache/storm/executor/bolt/BoltExecutor.java#L109) no longer appear in the worker log. I also tested changing simply changing the log level in the child log4j2 config and that also works as expected. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacobtolar/storm STORM-3198 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2806.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 #2806 commit 28ad8dc77b409339768484839ab6979f42ce07ff Author: Jacob Tolar Date: 2018-08-15T19:53:42Z [STORM-3198] Topology submitters should be able to supply log4j2 configurations ---
[GitHub] storm pull request #2802: STORM-3194 reduce FIFOSchedulingPriorityStrategy l...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2802 ---
[GitHub] storm issue #2792: Add the getName() method in order to obtain the applied l...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2792 The changes look good to me. Please raise an issue at https://issues.apache.org/jira and rename this PR and the commit message to contain the issue number. ---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2805#discussion_r210688568 --- Diff: pom.xml --- @@ -280,7 +280,7 @@ 1.6.6 2.8.2 1.7.21 -3.1.0 +3.2.6 --- End diff -- Raised https://issues.apache.org/jira/browse/STORM-3199 to get rid of metrics-ganglia ---
[GitHub] storm pull request #2800: STORM-3162: Fix concurrent modification bug
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2800#discussion_r210639246
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -1660,26 +1658,18 @@ private TopologyDetails readTopologyDetails(String
topoId, StormBase base) throw
private void updateHeartbeatsFromZkHeartbeat(String topoId,
Set> allExecutors, Assignment existingAssignment) {
LOG.debug("Updating heartbeats for {} {} (from ZK heartbeat)",
topoId, allExecutors);
-IStormClusterState state = stormClusterState;
Map, Map> executorBeats =
-StatsUtil.convertExecutorBeats(state.executorBeats(topoId,
existingAssignment.get_executor_node_port()));
-Map, Map> cache =
StatsUtil.updateHeartbeatCacheFromZkHeartbeat(heartbeatsCache.get().get(topoId),
-
executorBeats, allExecutors,
-
ObjectReader.getInt(conf.get(
-
DaemonConfig
-
.NIMBUS_TASK_TIMEOUT_SECS)));
-heartbeatsCache.getAndUpdate(new Assoc<>(topoId, cache));
+
StatsUtil.convertExecutorBeats(stormClusterState.executorBeats(topoId,
existingAssignment.get_executor_node_port()));
+heartbeatsCache.compute(topoId, (k, v) ->
+//Guaranteed side-effect-free
--- End diff --
We should probably put this requirement in comments on the two methods in
StatsUtil rather than here, so they stay side effect free.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user jacobtolar commented on a diff in the pull request: https://github.com/apache/storm/pull/2806#discussion_r210671831 --- Diff: pom.xml --- @@ -278,7 +278,7 @@ 4.1.25.Final 1.0.2 1.6.6 -2.8.2 +2.11.0 --- End diff -- Note: The update to log4j2-2.11.0 is not strictly necessary. 2.8.2 supports composite configuration. However, there was [a bug](https://issues.apache.org/jira/browse/LOG4J2-2123) in merging filters from child configurations that I got fixed in 2.11.0. If a child configuration added filters and there were none in the parent, the child filter would be ignored. I would like to add some filters in my topology, so, bumping the version to the one with the fix. ---
[GitHub] storm issue #2803: STORM-2578: Apply new code style to storm-elasticsearch
Github user milantracy commented on the issue: https://github.com/apache/storm/pull/2803 Hi @HeartSaVioR , reduced the value to 0. ---
[GitHub] storm pull request #2807: STORM-3199: Remove metrics-ganglia due to LGPL dep...
GitHub user srdo opened a pull request: https://github.com/apache/storm/pull/2807 STORM-3199: Remove metrics-ganglia due to LGPL dependency https://issues.apache.org/jira/browse/STORM-3199 You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/storm STORM-3199 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2807.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 #2807 commit 806bb11ca26b5caa41c6d0ef7c4658855219cec3 Author: Stig Rohde Døssing Date: 2018-08-16T18:04:11Z STORM-3199: Remove metrics-ganglia due to LGPL dependency ---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user jacobtolar commented on a diff in the pull request: https://github.com/apache/storm/pull/2806#discussion_r210769170 --- Diff: pom.xml --- @@ -278,7 +278,7 @@ 4.1.25.Final 1.0.2 1.6.6 -2.8.2 +2.11.0 --- End diff -- not aware of any such reason; done ---
[GitHub] storm issue #2806: [STORM-3198] Topology submitters should be able to supply...
Github user jacobtolar commented on the issue: https://github.com/apache/storm/pull/2806 Looks like there are a couple unit tests I need to fix. Probably won't get to that till next week. ---
Re: [DISCUSS] Plans for releasing Storm 1.2.3
+1 On Wed, Aug 8, 2018 at 1:16 AM Hugo Louro wrote: > +1 > > On Tue, Aug 7, 2018 at 9:02 AM Alexandre Vermeerbergen < > avermeerber...@gmail.com> wrote: > > > +1 for a Storm release 1.2.3 first (non binding) > > > > Alexandre Vermeerbergen > > Le mar. 7 août 2018 à 17:47, Stig Rohde Døssing > > a écrit : > > > > > > Hi devs, > > > > > > I've seen a couple of requests for releasing Storm 1.2.3 soon on the > > users > > > list. I know we're currently working toward a release for Storm 2.0.0, > > but > > > I think it would be nice to release 1.2.3 first. It contains some > decent > > > bugfixes, and also contains the deprecation of storm-druid. Last > release > > > was a couple of months ago, so I think it's a good time to do another. > > > > > > What do you think? Is there anything pending we should get in first? > > > -- *Aniket Alhat*
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210768599
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java
---
@@ -622,6 +620,12 @@ protected String javaCmd(String cmd) {
List classPathParams = getClassPathParams(stormRoot,
topoVersion);
List commonParams = getCommonParams();
+String log4jConfigurationFile = getWorkerLoggingConfigFile();
--- End diff --
No, it can't -- The xml file might be in the topology jar uploaded to the
nimbus. That file is not on the classpath if the LogWriter feature is used.
So I guess it could, but getCommonParams would have to take a parameter and
get invoked twice. Right now it's just invoked once and the output is used for
both LogWriter and Worker jvm startup scripts.
---
