Github user hmcl commented on a diff in the pull request:
https://github.com/apache/storm/pull/1826#discussion_r92919073
--- Diff:
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
---
@@ -77,7 +77,7 @@
private KafkaSpoutStreams kafkaSpoutStreams; //
Object that wraps all the logic to declare output fields and emit tuples
private transient KafkaSpoutTuplesBuilder<K, V> tuplesBuilder; //
Object that contains the logic to build tuples for each ConsumerRecord
- private transient Map<TopicPartition, OffsetEntry> acked; //
Tuples that were successfully acked. These tuples will be committed
periodically when the commit timer expires, after consumer rebalance, or on
close/deactivate
+ transient Map<TopicPartition, OffsetEntry> acked; // Tuples
that were successfully acked. These tuples will be committed periodically when
the commit timer expires, after consumer rebalance, or on close/deactivate
--- End diff --
@srdo using reflection for tests only wouldn't look to concerning to me,
but I also agree with your point of not being able to do find usages in the
IDE. I would also say that if the tests and the source class are correctly
implemented, the tests should never need to query the internal state to assert
of the correctness of the code. If one has to do that, that to me feels like
something is not as good as it could be.
In my opinion package private methods have a very specific use, which is
for the construction of libraries. Although they are not part of the public
API, they are easy to misuse by clients who may want to take shortcuts. With
this said, I think that having a package where we put some of the nested
classes would make the code significantly more modular. I really think that it
is getting to the point where I start seeing `if / else` blocks all over the
place, added to `instance of` ... and I really want to avoid that at all
costs... otherwise soon we wont' have a hand on this.
However extracting the nested classes will also add an extra complexity,
because there is a significant amount of state that the `OffsetEntry` class
uses that would have to be passed around if it becomes it's own class. I still
thing we should do it, though. When we do it should be in such a way that we
can plugin a different implementation of `OffsetEntry`. Perhaps also rename
this class, as it is really an `OffsetManager`. What do you think ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
