STORM-2568: Fix getTopicsString #2177
Conversation
haewanj
commented
Jun 24, 2017
haewanj
commented
- String.valueOf returns square bracket, because 'topics' is Collection
- removed square bracket to use storm-kafka-monitor
|
+1 |
|
@haewanj |
- String.valueOf returns square bracket, because topics is Collection - removed square bracket to use storm-kafka-monitor
|
@HeartSaVioR |
| @@ -56,6 +57,6 @@ public NamedSubscription(String ... topics) { | |||
|
|
|||
| @Override | |||
| public String getTopicsString() { | |||
| return String.valueOf(topics); | |||
| return StringUtils.join(topics, ","); | |||
There was a problem hiding this comment.
I would like to suggest that you use instead Guava's Joiner.on(",").join(topics); which will avoid adding a new dependency
There was a problem hiding this comment.
Thanks for the suggestion.
As you know, To keep java 1.7+ build, 'String.join()' is not a solution like master branch. And 'storm-core' has dependency both commons-lang and guava. So I choose commons-lang but If to pass the test that travis MODUELS='!storm-core', storm-kafka-client needs the dependency even Guava too. That's why I added commons-lang to storm-kafka-client's pom file.
If avoid adding a new dependency in storm-kafka-client, we can code maybe like this
StringBuilder sb = new StringBuilder();
for(String topic: topics) {
sb.append(topic).append(",");
}
sb.deleteCharAt(sb.lastIndexOf(","));
return sb.toString();
Many of external/packages have commons-lang dependency so, I thought it was fine.
By the way It has not been passed the Integration-test on travis-ci, I have no idea. but in my local integration-test is fine. (ex: mvn -P integration-tests-only integration-test)
|
+1 |
|
+1 |
