Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo Thanks for the review. I have squashed the commits and it is ready to
merge.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2465
+1
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo I would like to merge this patch asap. Can you please take a quick
look such that I can squash and merge it in. Thanks.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo done. Pls check and I will squash the commits right away. I would
like to try to merge this in today. I will update the master PR with everything
squashed already. Thanks.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo It should be OK now. If you have further requests lest's file a
refactoring JIRA and include in it refactoring some of the unit tests for
better code reuse.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo can you pls take a look. Thanks.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2465
Thanks, I missed those changes.
Regarding testing EARLIEST/LATEST, I don't understand why you'd need to
mock KafkaSpoutConfig. I think it can be tested as an integration test, like
the ones in
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
These two changes address your comment: "_test that when you call
findNextCommitOffset, it returns the metadata you put in)_"
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo can you please do one last review. Thanks.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo it should be good now. Can you please take a look. Thanks.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2465
@hmcl There were a few more comments, just making sure they weren't missed.
https://github.com/apache/storm/pull/2465#discussion_r157350384
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo once I get +1 I will create the PR for master.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo I am working on making unit tests pass and then will squash and
create master PR. Can you take another look in the meantime. Thanks.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2465
@hmcl Great, thanks.
---
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo I created the PR here because some topologies with latest master were
not working for me and I worked off 1.x-branch to be able to make progress. I
will submit a PR for master as well.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2465
@hmcl I don't know that there's a hard rule that it has to be that way, but
it seems like a good way to me to ensure that master is the "most fixed"/best
state of the code. It's been done this way in
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo I plan on submitting a patch to master, but why does the patch need
to go on master first? For example, there could be bugs in 1.x that no longer
exist in master because of other changes, and in
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2465
@srdo can you please do a first pass review. In the meantime I will address
the 2 or 3 TODO with minor cleanups. The following tests are also failing. I am
working on fixing them, but if you know from
18 matches
Mail list logo
