[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 @srdo don't worry about the authorship. It was just a code review for which I created a PR to make it easier to discuss. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2428 Squashed to one commit. Not entirely sure if this is okay, since authorship is lost for your changes, but maybe it's not a big deal? ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 +1. Can you please squash. Thanks. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2428 @hmcl Thanks, merged your PR and squashed to two commits. The only change since your PR is a whitespace change, and removing an unnecessary else (there was a throw in the if block) ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 @srdo although some of these methods have been deprecated for 2.0, customers that are currently in a 1.x.y version will likely use this version for a few years. We will have to maintain this codebase for quite a long time, and therefore I am in favor of making at least the code a bit more readable. I had quite a hard time to understand what the existing code is doing. I have another suggestion, which I also shared with you on a [PR](https://github.com/srdo/storm/pull/1). I will leave it up to you which one to pick and I am +1 after that such that we can move forward. Thanks. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 @srdo aiming to getting this PR merged more quickly I created a [PR](https://github.com/srdo/storm/pull/1) with a suggested fix off your branch. If you agree with the fix, can you please incorporate it, squash the commits, and push it again here. I will then review it right away. Thanks. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 @arunmahadevan I am looking into this now. Thanks. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2428 @HeartSaVioR can we look at merging this? @hmcl if you have any further comments you can put it here asap. ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2428 @HeartSaVioR @srdo I will finalize my review by Monday PST ---
[GitHub] storm issue #2428: STORM-2826: Set key/value deserializer fields when using ...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2428 +1 @hmcl It would be great if you could finish the review, sure, in several days after Thanksgiving week. ---
