[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1832 +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 Squashed this and pushed 1.x version at https://github.com/apache/storm/pull/1941. Thanks all for good feedback :) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1832 +1 for all the KafkaSpout related code +1 also for the SimulatedTime changes in the sense that the minor refactoring of this class does not change the semantics of the pre-existing code. @srdo can you please squash all the commits and create a PR for 1.x-branch, such that we can backport these changes. Thanks. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 @HeartSaVioR Fixed conflict --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1832 @srdo Could you rebase this? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 Thanks, both of you :) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1832 @srdo I was also caught up in a couple of things. I will finish reviewing this tonight. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1832 @srdo for Time and Timer for a pure correctness perspective the changes look good to me. I don't see any issues. I took a quick look at the rest of the patch and it too looks good, but I don't dig in to it so I don't feel comfortable giving a +1 at this point. I would love to really take a look at it, but I am sadly swamped with other things right now. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 @revans2 I agree, it is best to keep nanos and millis separate. I've updated Time to pass through to System.currrentTimeMillis for millisecond methods, and System.nanoTime for nanosecond methods. When simulating everything gets converted to nanos. @HeartSaVioR I agree, I'm not really seeing a use case for submillisecond support in storm-kafka-client, because the things we're currently doing with timers is to set retry backoffs or commit periods. I don't think anyone would want either to be less than 1 millisecond. @hmcl Is there a reason nano/microsecond support is needed? I've updated Time and Timer to support nanoseconds (hopefully without introducing bugs). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1832
Let's back to actual usage of Timer.
```
if (!consumerAutoCommitMode) { // If it is auto commit, no need
to commit offsets manually
commitTimer = new Timer(500,
kafkaSpoutConfig.getOffsetsCommitPeriodMs(), TimeUnit.MILLISECONDS);
}
refreshSubscriptionTimer = new Timer(500,
kafkaSpoutConfig.getPartitionRefreshPeriodMs(), TimeUnit.MILLISECONDS);
```
Here we are taking period as milliseconds consistently, and dropping
support nanoseconds on Timer means that up to 1 ms can be added to the period.
Unless we're expecting users to set these config values to small value like 1
or so, I think it doesn't matter. Indeed we're having 500 ms as default value,
which doesn't matter it will be 501 ms.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1832 @srdo Sorry I am kind of coming into this half way through. History: System.nanoTime and System.currentTimeMillis have different uses and are not on the same time line. nanoTime is guaranteed to be monotonically increasing so long as the box is up. currentTimeMillis is not, because it is kept in sync with the system time. currentTimeMillis on most OSes is a very cheap. It is reading from shared memory, no system call needed. It can be different if read from different threads even. nanoTime, at least on x86 boxes end up reading a special register in the processor. If there is a very small core count this is cheap. However if you are on a system with many different cores or physical chips they all have to be synced up to guarantee that it is monotonically increasing so it ends up being about as expensive as a memory barrier. Not super expensive but also not dead cheap. As far as Time.java is concerned. Simulated time can be whatever we want. I don't care if we store nanos internally or millis internally. It is all simulated anyways. If we are not simulating I don't think we should mix the two or have one backed by the other. They are separate for a reason and people most of the time will pick one or the other because of those differences. If we want nano support then lest have Time support nano, but the milli APIs stay backed by System.currentTimeMillis, and the nano APIs are backed by System.nanoTime. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 @hmcl I gave updating Time to support nanoseconds a shot. Not really sure I'm happy with it, since it requires that we also switch the current millisecond support over to using System.nanoTime (System.nanoTime and System.currentTimeMillis are not on the same timeline, and are incomparable). If we're going to keep this change, Time.currentTimeMillis should be changed to a different name, so it doesn't imply that it's the same as System.currentTimeMillis. @revans2 Do you have an opinion on nanosecond support in `o.a.s.u.Time`? (asking because you have a number of commits on the Time file) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1832 @srdo everything looks OK for me. I just would rather leave the `Timer` in ns than in ms, and change the `SimulatedTime` class to support nanos. I think that moving forward that will be useful for other developers as well. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 @hmcl Updated. This should be ready for review. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1832: STORM-2250: Kafka Spout Refactoring to Increase Modularit...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1832 @hmcl Renamed. I'll wait until https://github.com/apache/storm/pull/1808 has been merged to fix conflicts here, since it's likely there's some overlap --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
