Skip to content

Conversation

@Renkai
Copy link
Contributor

@Renkai Renkai commented Aug 26, 2015

No description provided.

@harshach
Copy link
Contributor

@Renkai whats the need for this? why can't you add multiple bolts with its own parallelism. IMO this goes against the storm api.

@Renkai
Copy link
Contributor Author

Renkai commented Aug 26, 2015

@harshach
Different KafkaBolts may need to send topic to different kafka clusters,use different serializers,and have different params for performance tunning.
In current version there is no way to do that.

@revans2
Copy link
Contributor

revans2 commented Aug 26, 2015

The changes look fine to me. +1

@harshach we did a similar thing for the HDFS Bolt.

public HdfsBolt withConfigKey(String configKey){
this.configKey = configKey;
return this;
}

Each bolt needs to be able to be configured independently of all other bolts. Putting it in the conf is a is one way to do it, but I have seen a lot of bolts do it through a setter too. I personally would prefer to support both through a config and through an API. The only trick is with consistency and ordering in the case of conflicts.

@Parth-Brahmbhatt
Copy link
Contributor

+1.

@harshach
Copy link
Contributor

@revans2 @Renkai makes sense. Instead of using constructor can we should with Options like other bolts are using. probably "withProducerProperties" method and remove the topology level configs so that there isn't two ways configure the bolt.

@harshach
Copy link
Contributor

I am fine with the current approach but we need to standardize one way configuring the bolts as each connector follows its own pattern right now and which can be confusing to the users.

@Renkai
Copy link
Contributor Author

Renkai commented Aug 27, 2015

@harshach I will push a new commit,make it harmonic with current code.

@Renkai
Copy link
Contributor Author

Renkai commented Aug 27, 2015

Travis CI shows test fails in with jdk8,but I think it's not problems with my commits,tests all passed on my Mac.

@harshach
Copy link
Contributor

@Renkai those failed tests are intermittent test failures in storm-core not related to the patch.

@HeartSaVioR
Copy link
Contributor

+1.

@asfgit asfgit merged commit c5d77ac into apache:master Sep 1, 2015
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.

6 participants