Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Add null check when heron reads the config file #1682

Merged
merged 4 commits into from
Feb 2, 2017
Merged

Conversation

huijunw
Copy link
Contributor

@huijunw huijunw commented Jan 31, 2017

add null check when Heron reads the config file. resolves #1681

@huijunw huijunw requested a review from maosongfu January 31, 2017 19:55
@huijunw
Copy link
Contributor Author

huijunw commented Jan 31, 2017

origin 1861 link
#1681

@huijunw huijunw changed the title #1681 fix #1681 fix: added null check when heron reads the config file Jan 31, 2017
@objmagic objmagic changed the title #1681 fix: added null check when heron reads the config file Add null check when heron reads the config file Jan 31, 2017
@@ -66,7 +66,7 @@ public void dump(TopologyAPI.Component.Builder bldr) {
for (Map.Entry<String, Object> entry : componentConfiguration.entrySet()) {
TopologyAPI.Config.KeyValue.Builder kvBldr = TopologyAPI.Config.KeyValue.newBuilder();
kvBldr.setKey(entry.getKey());
kvBldr.setValue(entry.getValue().toString());
kvBldr.setValue(entry.getValue() == null ? "null" : entry.getValue().toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put null in there instead of "null".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@objmagic objmagic added the bug label Jan 31, 2017
@billonahill
Copy link
Contributor

👍

1 similar comment
@objmagic
Copy link
Contributor

objmagic commented Feb 1, 2017

👍

@maosongfu
Copy link
Contributor

Did you test it locally with null config-value? @huijunw

@huijunw
Copy link
Contributor Author

huijunw commented Feb 2, 2017

updated. tested.

For the null value in the map, there are 4 options:

  1. put null there. exception thrown and test failed.
    Neither protobuf nor heron definition allows null value config.
  2. put "" there.
  3. put "null".
    The string semantic is inconsistent within heron during parsing process.
  4. ignore that config entry.
    The null value basically means the entry is not a meaningful config. A warning is logged.

Thanks for @maosongfu suggestion.

@billonahill
Copy link
Contributor

👍

@huijunw huijunw self-assigned this Feb 2, 2017
@@ -64,6 +66,10 @@ public void dump(TopologyAPI.Component.Builder bldr) {

TopologyAPI.Config.Builder cBldr = TopologyAPI.Config.newBuilder();
for (Map.Entry<String, Object> entry : componentConfiguration.entrySet()) {
if (entry.getValue() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check entry.getKey() too since the key can also be null.
https://docs.oracle.com/javase/7/docs/api/java/util/Map.html#put(K,%20V)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@maosongfu
Copy link
Contributor

Looks good to me. 👍

@huijunw huijunw merged commit 9a712d3 into apache:master Feb 2, 2017
@huijunw huijunw deleted the 1681 branch March 10, 2017 19:36
@huijunw huijunw assigned huijunw and unassigned huijunw Apr 5, 2017
nicknezis pushed a commit that referenced this pull request Sep 14, 2020
Add null check when heron reads the config file
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle null config case
4 participants