MINOR: Add Timer to simplify timeout bookkeeping by hachikuji · Pull Request #5087 · apache/kafka

hachikuji · 2018-05-27T18:59:22Z

This is an attempt to find a better pattern for blocking methods with a timeout. We currently do a lot of bookkeeping for timeouts which is both error-prone and distracting.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

guozhangwang · 2018-05-29T16:50:44Z

retest this please

hachikuji · 2018-05-29T19:45:56Z

One of the tests is failing and causing the controller-event-thread to be left around. I will investigate.

ijuma · 2018-05-29T23:26:45Z

Always verifying that your reviewers are paying attention. ;)

Just keeping you on your toes 😉.

hachikuji · 2018-05-30T00:06:23Z

oops. This should still be volatile.

hachikuji · 2018-05-30T08:05:54Z

retest this please

hachikuji · 2018-05-31T15:23:26Z

retest this please

lindong28 · 2018-06-03T07:19:25Z

@hachikuji This is a very nice improvement! If you need, I can help give the first around of review once it is ready.

ijuma

Thanks for the PR. A few initial comments/questions before doing a more thorough review.

ijuma · 2018-06-02T05:40:29Z

Maybe we can add a default method for hiResClockMs as well.

Also, is this method worth it? It seems like it saves us new and a comma.

time.timer(5000) new Timer(time, 5000)

Ack on the first point. For the second, I had initially expected it would be useful to be able to override the method in testing to provide an alternative implementation. I haven't needed it yet, but I'm slightly inclined to keep the option open and avoid the direct dependence on the Timer implementation. I also thought it might make Timer a bit more discoverable since most are familiar with the Time interface.

Shall we add javadoc to both of these methods?

ijuma · 2018-06-03T08:30:42Z

Should we have a method that can be used without negation? isValid(), isActive(), something else?

We could. I had considered adding a notExpired method since words like "active" and "valid" are a little ambiguous.

ijuma · 2018-06-03T08:42:26Z

It would be better to build something like this on top of time.nanoseconds() since it is monotonic. The downside is that it can't be used in places where a real date/time is expected. Would the latter be an issue?

At the moment, I've only updated the consumer code to use Timer. Lower level code like NetworkClient still have APIs which depend on the current time in milliseconds. In a future patch, I think we can change this code to use Timer as well. The ultimate goal would be to have a Timer interface which only exposed durations of times and did not have a direct dependence on wall clock time. Then we could safely use nanoseconds internally.

time.nanoseconds() may be more costly than milliseconds on some OS though.

Is this based on real data or hypothetical? :) I shared a link somewhere that shows that this is not true for environments we care about.

Well, I did not do any experiments myself, so I'd say it's hypothetical :P It's mainly from some articles I've read before.

http://pzemtsov.github.io/2017/07/23/the-slow-currenttimemillis.html

@ijuma thanks!

ijuma · 2018-06-20T13:50:56Z

I started reviewing the code and another high-level question occurred to me: should the Timer accept Duration instances?

Yeah, we can do that. I thought about it at one point, but everything was already so millisecond-centric.

A meta comment: this maybe related to #5183, we should let the contributor know about this and whether that PR can be refactored or dropped because of this.

ijuma · 2018-06-29T06:07:01Z

Sorry, can you please fix the merge conflicts?

ijuma · 2018-06-29T06:13:39Z

Seems like we need to update the comment above. Also, why are we replacing the two calls by a single call everywhere in this test?

I removed them because we could get the same effect with a single call and a non-zero timeout. Ack on removing the comment.

ijuma · 2018-06-29T06:14:39Z

Shall we add javadoc to both of these methods?

guozhangwang

Made a pass over non-testing code.

Could we trigger a streams simple benchmark (/tests/kafkatest/benchmarks/streams) just to validate there is no perf regressions?

guozhangwang · 2018-08-02T17:54:38Z

Thank you Java8 :)

guozhangwang · 2018-08-02T17:58:12Z

guozhangwang · 2018-08-02T18:00:27Z

A meta comment: this maybe related to #5183, we should let the contributor know about this and whether that PR can be refactored or dropped because of this.

guozhangwang · 2018-08-02T18:03:41Z

time.nanoseconds() may be more costly than milliseconds on some OS though.

guozhangwang · 2018-08-02T18:04:27Z

It is still preserved, right?

Let me clarify the comment. I was trying to say that the argument is ignored if it is not a monotonic update.

Ah, okay :)

guozhangwang · 2018-08-02T18:47:17Z

This is a bit overkill, but I agree it is cleaner..

guozhangwang · 2018-08-02T19:05:46Z

Why changing if to a while, and also add a client.poll()?

I think the point of this check was to allow some time for pending async commits to return, but the previous code seemed a little bizarre. What was the point of ensuring the coordinator is ready and then immediately closing? It made more sense to turn this into a loop and call poll so that we could give the OffsetCommit responses a chance to be delivered.

makes sense.

guozhangwang · 2018-08-02T19:10:20Z

I'm a bit concerned about the perf implication here: we call an additional time.millis every time with consumer.poll() now, could we still pass in now and add a reset function that accepts the currentTime like the overloaded update as well?

I think we're probably still ahead in terms of total calls to time.milliseconds, but I can try to remove this one also.

guozhangwang · 2018-08-02T19:11:20Z

Nice catch :)

guozhangwang · 2018-08-02T19:13:43Z

Why move this map into the while loop?

Good catch.

guozhangwang · 2018-08-03T17:07:15Z

@hachikuji The updates LGTM. Please feel free to merge after Jenkins passed.

guozhangwang · 2018-08-03T17:31:20Z

Triggered https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1891 for simple streams benchmark.

guozhangwang · 2018-08-10T21:39:29Z

            if (!parts.isEmpty())
                return parts;

+            Timer timer = time.timer(requestTimeoutMs);


@hachikuji I found this issue overlooked during the review: it should be timeout here..

MINOR: Add Timer to simplify timeout bookkeeping and use it in the consumer (apache#5087) We currently do a lot of bookkeeping for timeouts which is both error-prone and distracting. This patch adds a new `Timer` class to simplify this logic and control unnecessary calls to system time. In particular, this helps with nested timeout operations. The consumer has been updated to use the new class. Reviewers: Ismael Juma <ismael@juma.me.uk>, Guozhang Wang <wangguoz@gmail.com>

* cherry pick of: fc5f6b0 MINOR: Add Timer to simplify timeout bookkeeping and use it in the consumer (apache#5087) We currently do a lot of bookkeeping for timeouts which is both error-prone and distracting. This patch adds a new `Timer` class to simplify this logic and control unnecessary calls to system time. In particular, this helps with nested timeout operations. The consumer has been updated to use the new class. Reviewers: Ismael Juma <ismael@juma.me.uk>, Guozhang Wang <wangguoz@gmail.com> * Restore sensor functionality that was removed from patch.

hachikuji force-pushed the minor-add-common-timer branch 2 times, most recently from d9a68fc to 627d418 Compare May 28, 2018 07:04

hachikuji changed the title ~~MINOR: [WIP] Add Timer to simplify timeout bookkeeping~~ MINOR: Add Timer to simplify timeout bookkeeping May 28, 2018

guozhangwang mentioned this pull request May 29, 2018

KAFKA-5697: prevent poll() from blocking forever #4855

Merged

3 tasks

ijuma reviewed May 29, 2018

View reviewed changes

hachikuji commented May 30, 2018

View reviewed changes

hachikuji mentioned this pull request May 30, 2018

MINOR: Add upgrade notes for new consumer poll #5084

Merged

3 tasks

hachikuji force-pushed the minor-add-common-timer branch 2 times, most recently from 643f378 to 6688004 Compare May 31, 2018 07:53

ijuma reviewed Jun 3, 2018

View reviewed changes

hachikuji force-pushed the minor-add-common-timer branch from 6688004 to dc92ea5 Compare June 4, 2018 23:28

hachikuji mentioned this pull request Jun 14, 2018

KAFKA-6948 - Change comparison to avoid overflow inconsistencies #5183

Closed

3 tasks

hachikuji force-pushed the minor-add-common-timer branch from 5a73c37 to 7324e67 Compare June 14, 2018 21:58

ijuma self-assigned this Jun 18, 2018

ijuma reviewed Jun 20, 2018

View reviewed changes

ijuma reviewed Jun 29, 2018

View reviewed changes

hachikuji force-pushed the minor-add-common-timer branch from cf9919e to 1913326 Compare June 29, 2018 16:33

guozhangwang reviewed Aug 2, 2018

View reviewed changes

hachikuji force-pushed the minor-add-common-timer branch from 1913326 to e6dca32 Compare August 3, 2018 16:48

hachikuji added 3 commits August 3, 2018 13:46

MINOR: Add Timer to simplify consumer timeout bookkeeping

bf624cc

Add timer overload using Duration

87c4e67

Add notExpired function and address miscellaneous comments

c7768d8

hachikuji added 3 commits August 3, 2018 13:46

Address Guozhang's review comments

b942cf1

Add javadoc for reset() method

e4f4a7d

Fix breakage after rebase

0ef4698

hachikuji force-pushed the minor-add-common-timer branch from c4f2b58 to 0ef4698 Compare August 3, 2018 20:47

hachikuji merged commit fc5f6b0 into apache:trunk Aug 4, 2018

guozhangwang reviewed Aug 10, 2018

View reviewed changes

chia7712 mentioned this pull request Feb 24, 2019

Minor: auto-commit-offset doesn't execute after the deadline #4794

Closed

3 tasks

smccauliff mentioned this pull request Apr 24, 2019

cherry pick of: fc5f6b0e46ff81302b3e445fed0cdf454c942792 linkedin/kafka#18

Merged

Conversation

hachikuji commented May 27, 2018

Committer Checklist (excluded from commit message)

Uh oh!

guozhangwang commented May 29, 2018

Uh oh!

hachikuji commented May 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji commented May 30, 2018

Uh oh!

hachikuji commented May 31, 2018

Uh oh!

lindong28 commented Jun 3, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang Aug 3, 2018 •

edited

Loading