Skip to content

STORM-3066: Implement support for using list elements in properties in FluxParser#3155

Merged
srdo merged 1 commit into
apache:masterfrom
efgpinto:STORM-3066
Nov 2, 2019
Merged

STORM-3066: Implement support for using list elements in properties in FluxParser#3155
srdo merged 1 commit into
apache:masterfrom
efgpinto:STORM-3066

Conversation

@efgpinto
Copy link
Copy Markdown

This MR adds support for accessing elements of list properties in configurations of FluxParser.
The reference of list items can be done as:

  # test variable substitution for list element
  list.element.property.target: ${a.list.property[0]}

Additionally, refactors the code a bit more efficient and lighter on memory.

Copy link
Copy Markdown
Contributor

@srdo srdo left a comment

Choose a reason for hiding this comment

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

Looks great, left a few comments for your consideration

Comment thread flux/flux-core/src/main/java/org/apache/storm/flux/parser/FluxParser.java Outdated
Comment thread flux/flux-core/src/main/java/org/apache/storm/flux/parser/FluxParser.java Outdated
Comment thread flux/flux-core/src/main/java/org/apache/storm/flux/parser/FluxParser.java Outdated
LOG.info("Performing property substitution.");
for (Object key : properties.keySet()) {
str = str.replace("${" + key + "}", properties.getProperty((String)key));
reader.lines().forEach(line -> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is minor, but I'm wondering if this could be made slightly easier to read using .map and .collect along with the joining Collector https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collectors.html#joining--

That way you wouldn't have to add the new line to the StringBuilder manually, and there'd be less to do in the else-cases where things aren't matched.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Better now I think. Can you take a look? 🙇

@srdo
Copy link
Copy Markdown
Contributor

srdo commented Nov 2, 2019

+1

@srdo srdo merged commit dfb9b55 into apache:master Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants