[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Github user knusbaum commented on the issue: https://github.com/apache/storm/pull/459 @krichter722 I would either rename that Jira and update the description a bit, or open a new one. Once you've done that, add the jira number into the PR title - look at other Storm PRs for examples. After that and the checkstyle stuff, I don't see anything else that should prevent this from going in. :+1: ---
[GitHub] storm pull request #459: avoiding rawtypes in IBold.prepare in order to allo...
Github user knusbaum commented on a diff in the pull request: https://github.com/apache/storm/pull/459#discussion_r175268334 --- Diff: external/storm-kafka/pom.xml --- @@ -57,7 +57,7 @@ maven-checkstyle-plugin -557 +561 --- End diff -- What we did with checkstyle, for better or worse, was agree on a set of rules and then apply those rules. We didn't go through and fix every checkstyle violation when we did that. However, we agreed that any pull requests should not increase the number of alerts, and preferably decrease them. It would be better if you could reformat any new 80+character lines to please the checkstyle monster. ---
[GitHub] storm pull request #2593: STORM-2994 KafkaSpout commit offsets for null tupl...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2593#discussion_r175256064
--- Diff:
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
---
@@ -484,8 +484,11 @@ private boolean emitOrRetryTuple(ConsumerRecord
record) {
return true;
}
} else {
+/*if a null tuple is not configured to be emitted, it
should be marked as emitted and acked immediately
+* to allow its offset to be commited to Kafka*/
LOG.debug("Not emitting null tuple for record [{}] as
defined in configuration.", record);
msgId.setEmitted(false);
--- End diff --
I'd like to see this property renamed to "isFiltered" or "isNullTuple"
something similar. Right now we have both a list of emitted tuples, and also
the isEmitted property, and they don't always match. It seems unnecessarily
confusing.
---
[GitHub] storm pull request #2593: STORM-2994 KafkaSpout commit offsets for null tupl...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2593#discussion_r175256133
--- Diff:
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
---
@@ -576,6 +579,8 @@ public void ack(Object messageId) {
+ "came from a topic-partition that this consumer
group instance is no longer tracking "
+ "due to rebalance/partition reassignment. No action
taken.", msgId);
} else {
+//a null tuple should be added to the ack list since by
definition is a direct ack
--- End diff --
We should probably switch the order of the emitted contains and msgId
isEmitted checks. It is not possible that isEmitted is false while the tuple is
in the emitted list.
---
[GitHub] storm pull request #2593: STORM-2994 KafkaSpout commit offsets for null tupl...
GitHub user reiabreu opened a pull request: https://github.com/apache/storm/pull/2593 STORM-2994 KafkaSpout commit offsets for null tuples Hello, Let's kick off this pull request. Unit tests for null tuples were missing. I'm in the process of adding them. I'll update this PR asap. Meanwhile, these seem to be the necessary changes You can merge this pull request into a Git repository by running: $ git pull https://github.com/reiabreu/storm master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2593.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2593 commit 1db31bcf784a69d3904f39d86d8987f3c9e5e4bd Author: Rui Abreu Date: 2018-03-16T12:50:11Z null tuples are now marked as emitted and marked as acked commit 59235e79d64bb0190bf1b9364066215f3650 Author: Rui Abreu Date: 2018-03-17T11:09:08Z Undoing import statements changes ---
