STORM-2826#1
Conversation
hmcl
commented
Dec 9, 2017
hmcl
commented
- Suggested Fix
- Suggested Fix
- Suggested Fix2
hmcl
force-pushed
the
1.x-branch_PR_STORM-2826_KVSerDe
branch
from
5ccf478 to
ed2c886
Compare
srdo
left a comment
srdo
left a comment
There was a problem hiding this comment.
Most of the changes seem good. Left a few comments.
| if (bootstrapServers == null || bootstrapServers.isEmpty()) { | ||
| throw new IllegalArgumentException("bootstrap servers cannot be null"); | ||
| } else { | ||
| setNonNullValProp(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, bootstrapServers, false); |
There was a problem hiding this comment.
It doesn't make sense to use setNonNullValProp here, we already know bootstrapServers isn't null.
| this(keyDes, keyDesClazz, valueDes, valueDesClazz, builder.subscription, | ||
| (RecordTranslator<K, V>) builder.translator, new HashMap<String, Object>()); | ||
|
|
||
| this.kafkaProps.putAll(builder.kafkaProps); |
There was a problem hiding this comment.
It's probably nicer to put this in the HashMap constructor call in line 300 instead
There was a problem hiding this comment.
I thought about that, but then i would have to use the anonymous class with methods, i.e. the following code, which a lot of people call an anti-pattern. I am fine either way. What do you think is best?
new HashMap<String,Object>(){{putAll(builder.kafkaProps);}}There was a problem hiding this comment.
I meant use the HashMap copy constructor https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#HashMap-java.util.Map-
|
|
||
| private void setNonNullValProp(String key, Object val, boolean isClass) { | ||
| if (val != null) { | ||
| if (isClass) { |
There was a problem hiding this comment.
I think you should rename the parameter. It's true if the val is not a class.
| } else if (valueDesClazz != null) { | ||
| this.kafkaProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, valueDesClazz); | ||
| } | ||
| setSerDeKafkaProps(keyDes, keyDesClazz, valueDes, valueDesClazz); |
There was a problem hiding this comment.
Can we add a note to this constructor that only one of keyDes, keyDesClazz, valueDes, valueDesClazz should be non-null at a time? It was part of the reason for the if-else-if-else above. I'd say we should add a check that only one is not null, but it's probably not going to be pretty so a comment is fine.
There was a problem hiding this comment.
Sure, I agree. But the reason I didn't put it is because in one of the constructors there was no if-else-if. We will be modifying the semantics slightly, but I think it is safe to do so. Agree?
There was a problem hiding this comment.
This constructor is private and only used by the setKey/setValue methods, so it's easy to check that it's never called incorrectly. The other constructor shouldn't have this rule.
- Suggested Fix2 - Address code reviews
|
@srdo can you please check now, and in case it is fine, incorporate these changes in the main patch in Apache Storm. Thanks. |
|
Looks good, thanks. |
2 participants
