[GitHub] storm issue #2365: [STORM-2773]If a drpcserver node in cluster is down,drpc ...

[liu-zhaokun]

Github user liu-zhaokun commented on the issue:

https://github.com/apache/storm/pull/2365
  
Hi,@revans2,I'm not sure whether the ci's failure related to me.And I have 
add a retry to DRPCInvaction,could you help me to review it? 


---

[GitHub] storm pull request #2433: [STORM-2693] Heartbeats and assignments promotion ...

[danny0405]

Github user danny0405 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2433#discussion_r153037892
  
--- Diff: storm-client/src/jvm/org/apache/storm/stats/StatsUtil.java ---
@@ -1589,18 +1570,16 @@ public static ComponentPageInfo aggCompExecsStats(
 if (lastReportedTime != null) {
 reportedTime = lastReportedTime;
 } else {
-reportedTime = 0;
+reportedTime = lastReportedTime = 0;
--- End diff --

we do not decide if updateHeartbeatCache is null when 
reportedTime.equals(lastReportedTime)


---

[GitHub] storm pull request #2433: [STORM-2693] Heartbeats and assignments promotion ...

[danny0405]

Github user danny0405 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2433#discussion_r153036973
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -43,13 +44,15 @@
 private static Logger LOG = 
LoggerFactory.getLogger(StormClusterStateImpl.class);
 
 private IStateStorage stateStorage;
+private ILocalAssignmentsBackend backend;
--- End diff --

there is also import org.apache.storm.generated.* here, should i make a 
change?


---

[GitHub] storm pull request #2433: [STORM-2693] Heartbeats and assignments promotion ...

[danny0405]

Github user danny0405 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2433#discussion_r153037176
  
--- Diff: 
storm-client/src/jvm/org/apache/storm/cluster/StormClusterStateImpl.java ---
@@ -147,21 +153,52 @@ protected void 
issueMapCallback(ConcurrentHashMap callbackConc
 
 @Override
 public List assignments(Runnable callback) {
+//deprecated
--- End diff --

i will just remove it


---

[GitHub] storm pull request #2433: [STORM-2693] Heartbeats and assignments promotion ...

[danny0405]

Github user danny0405 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2433#discussion_r153036746
  
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -1293,6 +1318,44 @@
 @isPositiveNumber
 public static final String SUPERVISOR_CPU_CAPACITY = 
"supervisor.cpu.capacity";
 
+@isInteger
--- End diff --

this supervisor config is used by SupervisorClient, so they can not be 
moved to storm-server


---

[GitHub] storm pull request #2428: STORM-2826: Set key/value deserializer fields when...

[HeartSaVioR]

Github user HeartSaVioR commented on a diff in the pull request:

https://github.com/apache/storm/pull/2428#discussion_r153035388
  
--- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java
 ---
@@ -320,20 +320,17 @@ private Builder(Builder builder, 
SerializableDeserializer keyDes, Class
 // when they change the key/value types.
 this.translator = (RecordTranslator) builder.translator;
 this.retryService = builder.retryService;
-
-if (keyDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDesClazz);
-}
+
 if (keyDes != null) {
 
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, 
keyDes.getClass());
-}
-if (valueDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, 
valueDesClazz);
-}
-if (valueDes != null) {
+} else if (keyDesClazz != null) {
--- End diff --

OK. That doesn't looks like easy to understand, but I understand that's 
unavoidable for backward compatibility.


---

[GitHub] storm pull request #2428: STORM-2826: Set key/value deserializer fields when...

[srdo]

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2428#discussion_r153035015
  
--- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java
 ---
@@ -320,20 +320,17 @@ private Builder(Builder builder, 
SerializableDeserializer keyDes, Class
 // when they change the key/value types.
 this.translator = (RecordTranslator) builder.translator;
 this.retryService = builder.retryService;
-
-if (keyDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDesClazz);
-}
+
 if (keyDes != null) {
 
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, 
keyDes.getClass());
-}
-if (valueDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, 
valueDesClazz);
-}
-if (valueDes != null) {
+} else if (keyDesClazz != null) {
--- End diff --

One of the changes in https://github.com/apache/storm/pull/2215 is to make 
users set most Kafka properties through setProp instead of duplicating 
configuration parameters in KafkaSpoutConfig. These properties include key and 
value deserializers.

In order to provide backward compatibility that PR tries to ensure that if 
the keyDes/keyDesClazz field is set, then the corresponding kafkaProps property 
is also set. The spout doesn't use the fields anymore, it only reads from the 
kafkaProps map. The setKey/setValue methods were also deprecated, and users 
were directed to use setProp instead. Additionally the convenience builder 
methods were changed so they didn't set the keyDes/keyDesClazz fields, but just 
set properties in kafkaProps instead. 

The issue Alexandre hit during testing was that he was doing something like 
`kafkaSpoutConfig.getKeyDeserializer().getClass()` on a KafkaSpoutConfig built 
from one of the convenience builders. Since keyDes/keyDesClazz aren't being set 
by the default builders anymore, this causes an NPE.

I think we still want to be able to provide the simplified KafkaSpoutConfig 
API to 1.x, and we still want to encourage users to use setProp instead of 
setKey/setValue. In order to fix the NPE we also have to set keyDes/keyDesClazz 
in the convenience builder methods again.

Without this change the code behaves in a very confusing way when mixing 
use of setKey/setValue and setProp. The added unit tests demonstrate cases 
where the current code would act counterintuitively. Try pasting the current 
code into the modified Builder constructor, and you'll see test failures.

The modified constructor is used when setKey/setValue is called. With the 
current code, calling e.g. setKey will actually overwrite both the key and 
value properties in kafkaProps if keyDesClazz and valueDesClazz were set in the 
old Builder. 

For example, if you did 
`KafkaSpoutConfig.builder(topic).setProp(MyValueDeserializer.class).setKey(MyKeyDeserializer.class)`,
 you end up with a KafkaSpoutConfig where the value deserializer is 
`StringDeserializer`. This is because the `builder` method now sets both 
keyDesClazz and valueDesClazz by default. When setKey calls the copy 
constructor, it overwrites both the key and value deserializer properties in 
kafkaProps with the values of the key/valueDesClazz fields. The updated code 
makes sure that if you call setKey, then only the key deserializer property 
will be set. 

The change isn't necessary for the constructor further up, because it isn't 
a copy constructor. The problem with the copy constructor is that it takes in a 
kafkaProps that already might contain settings for key/value deserializers and 
overwrites them. The unmodified constructor has an empty kafkaProps, so there's 
no issue.


---

[GitHub] storm pull request #2429: STORM-2813: Use a class for normalized resources n...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2429


---

[GitHub] storm pull request #2428: STORM-2826: Set key/value deserializer fields when...

[HeartSaVioR]

Github user HeartSaVioR commented on a diff in the pull request:

https://github.com/apache/storm/pull/2428#discussion_r153033776
  
--- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java
 ---
@@ -320,20 +320,17 @@ private Builder(Builder builder, 
SerializableDeserializer keyDes, Class
 // when they change the key/value types.
 this.translator = (RecordTranslator) builder.translator;
 this.retryService = builder.retryService;
-
-if (keyDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDesClazz);
-}
+
 if (keyDes != null) {
 
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, 
keyDes.getClass());
-}
-if (valueDesClazz != null) {
-
this.kafkaProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, 
valueDesClazz);
-}
-if (valueDes != null) {
+} else if (keyDesClazz != null) {
--- End diff --



I'm not sure why this change is necessary, and if change is necessary, why 
we don't change above constructor as well?



---

[GitHub] storm issue #2432: [STORM-2829] fix logviewer deepSearch direct self-referen...

[srdo]

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2432
  
Thanks @Ethanlm, merged to master


---

[GitHub] storm pull request #2432: [STORM-2829] fix logviewer deepSearch direct self-...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2432


---

[GitHub] storm pull request #2434: STORM-2830: Upgrade to Jackson 2.9.2. Switch to us...

[srdo]

GitHub user srdo opened a pull request:

https://github.com/apache/storm/pull/2434

STORM-2830: Upgrade to Jackson 2.9.2. Switch to using the Jackson BOM…

… to keep components in sync

The breaking change lists are here 
https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.9. Nothing jumped 
out at me as something we'd be affected by.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/srdo/storm STORM-2830

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2434.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 #2434


commit 6568fd066df04c18c42f39249cea9673f546c59a
Author: Stig Rohde Døssing 
Date:   2017-11-23T21:48:06Z

STORM-2830: Upgrade to Jackson 2.9.2. Switch to using the Jackson BOM to 
keep components in sync




---