Skip to content

MINOR: Only update a request's local complete time in API handler if unset.#7813

Merged
hachikuji merged 2 commits into
apache:trunkfrom
bdbyrne:metric-fix
Jan 22, 2020
Merged

MINOR: Only update a request's local complete time in API handler if unset.#7813
hachikuji merged 2 commits into
apache:trunkfrom
bdbyrne:metric-fix

Conversation

@bdbyrne

@bdbyrne bdbyrne commented Dec 10, 2019

Copy link
Copy Markdown
Contributor

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually seems like an intuitive time to set the local complete time. I guess the reason we set it in RequestChannel.Response also is that we are trying to set it consistently with responseCompleteTimeNanos. I can't really think of a better solution than what you have here, but it might at least be helpful to add a comment of explanation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this code and the way it works today is super confusing. :) I think this change makes sense. However, I was curious how you noticed this. Was there a delay in getting to this point somehow? In theory, the value at this point should be very similar to the value we set during processing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember, the value was close enough that I didn't consider it worth looking into. I encountered it when I added debug metrics to further subdivide the local time, and there appeared to be lost time, i.e. the sum wasn't adding to the whole.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.

@bdbyrne

bdbyrne commented Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

It's also updated from the ClientRequestQuotaManager when determining the throttle time. I added a general comment stating the somewhat-obvious that it's possible for it to be set during processing, but in the end, I don't think it's a big deal. I only noticed it when doing some profiling and finding some numbers were off in the calculations.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@hachikuji

Copy link
Copy Markdown
Contributor

retest this please

@hachikuji hachikuji merged commit b4ecd03 into apache:trunk Jan 22, 2020
ijuma added a commit to confluentinc/kafka that referenced this pull request Jan 23, 2020
* apache-github/trunk:
  KAFKA-9418; Add new sendOffsetsToTransaction API to KafkaProducer (apache#7952)
  KAFKA-7273 Clarification on mutability of headers passed to Converter#fromConnectData() (apache#7489)
  MINOR: Only update a request's local complete time in API handler if unset (apache#7813)
  KAFKA-9143: Log task reconfiguration error only when it happened (apache#7648)
  MINOR: Change the log level from ERROR to DEBUG when failing to get plugin loader for connector (apache#7964)
  KAFKA-9024: Better error message when field specified does not exist (apache#7819)
  KAFKA-7204: Avoid clearing records for paused partitions on poll of MockConsumer (apache#7505)
  KAFKA-9083: Various fixes/improvements for Connect's Values class (apache#7593)
  MINOR: log error message from Connect sink exception (apache#7555)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants