Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORM-2864: Minor optimisation about trident kafka state #2476

Merged
merged 1 commit into from Dec 22, 2017

Conversation

@OuYangLiang
Copy link
Contributor

commented Dec 20, 2017

Make TridentKafkaState a template class to eliminate warning messages in eclipse, and a minor optimisation that use StringBuilder.append instead of string concat operation.

@srdo
Copy link
Contributor

left a comment

Looks good, with a few nitpicks

@@ -73,7 +71,7 @@ public void commit(Long txid) {
public void prepare(Properties options) {
Objects.requireNonNull(mapper, "mapper can not be null");
Objects.requireNonNull(topicSelector, "topicSelector can not be null");
producer = new KafkaProducer(options);
producer = new KafkaProducer<K, V>(options);

This comment has been minimized.

Copy link
@srdo

srdo Dec 20, 2017

Contributor

Nit: I think new KafkaProducer<>(options) will work here


if (topic != null) {
if (messageFromTuple != null) {
Future<RecordMetadata> result = producer.send(new ProducerRecord(topic, keyFromTuple, messageFromTuple));
Future<RecordMetadata> result = producer.send(new ProducerRecord<K, V>(topic, keyFromTuple, messageFromTuple));

This comment has been minimized.

Copy link
@srdo

srdo Dec 20, 2017

Contributor

Pretty sure the <> will also work here

@@ -116,8 +114,9 @@ public void updateState(List<TridentTuple> tuples, TridentCollector collector) {
}

if (exceptions.size() > 0) {
StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages " + tuples + " from topic = " + topic
+ " because of the following exceptions:" + System.lineSeparator());
StringBuilder errorMsg = new StringBuilder("Could not retrieve result for messages ");

This comment has been minimized.

Copy link
@srdo

srdo Dec 20, 2017

Contributor

Since the message is only ~5 strings long, I don't think there's much reason to use a StringBuilder at all.

This comment has been minimized.

Copy link
@OuYangLiang

OuYangLiang Dec 21, 2017

Author Contributor

There's a foreach statement right below it to iterate the exceptions, and append each exception to the errorMsg, it is still necessary I think.

for (ExecutionException exception : exceptions) {
errorMsg = errorMsg.append(exception.getMessage()).append(System.lineSeparator());
}

This comment has been minimized.

Copy link
@srdo

srdo Dec 21, 2017

Contributor

Makes sense, thanks.

this.producerProperties = props;
return this;
}

@Override
public State makeState(Map<String, Object> conf, IMetricsContext metrics, int partitionIndex, int numPartitions) {
LOG.info("makeState(partitonIndex={}, numpartitions={}", partitionIndex, numPartitions);
TridentKafkaState state = new TridentKafkaState()
TridentKafkaState<K, V> state = new TridentKafkaState<K, V>()

This comment has been minimized.

Copy link
@srdo

srdo Dec 20, 2017

Contributor

Can use <> on the right hand side

@srdo

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

+1

@srdo

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

Ah, sorry I missed this in the first go round; You need to raise an issue at https://issues.apache.org/jira describing this change/issue, then rename this PR to reference the issue number (see for example the naming of #2477). Please also squash to one commit, with the commit message referencing the JIRA issue number.

@OuYangLiang OuYangLiang changed the title Minor optimisation about trident kafka state STORM-2864: Minor optimisation about trident kafka state Dec 22, 2017

STORM-2864: Minor optimisation about trident kafka state
Make TridentKafkaState a template class to eliminate warning messages in
eclipse, and a minor optimization that use StringBuilder.append instead
of string concat operation.

@OuYangLiang OuYangLiang force-pushed the OuYangLiang:master branch from 84434e5 to 74a718c Dec 22, 2017

@OuYangLiang

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

Done already.

@asfgit asfgit merged commit 74a718c into apache:master Dec 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@srdo

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

@OuYangLiang Thanks for contributing. Merged to master and 1.x-branch. Keep up the good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.