Skip to content

KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219#8567

Merged
ijuma merged 11 commits intoapache:trunkfrom
ijuma:kafka-9652-request-throttle-fix
Apr 30, 2020
Merged

KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219#8567
ijuma merged 11 commits intoapache:trunkfrom
ijuma:kafka-9652-request-throttle-fix

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Apr 27, 2020 •
edited
Loading

After KIP-219, responses are sent immediately and we rely on a combination
of clients and muting of the channel to throttle. The result of this is that
we need to track apiThrottleTimeMs as an explicit value instead of
inferring it. On the other hand, we no longer need
apiRemoteCompleteTimeNanos.

Extend BaseQuotaTest to verify that throttle time in the request channel
metrics are being set. Given the nature of the throttling numbers, the test
is not particularly precise.

I included a few clean-ups:

  • Pass KafkaMetric to QuotaViolationException so that the caller doesn't
    have to retrieve it from the metrics registry.
  • Inline Supplier in SocketServer (use SAM).
  • Reduce redundant time.milliseconds and time.nanosecondscalls.
  • Use monotonic clock in ThrottledChannel and simplify compareTo method.
  • Simplify TimerTaskList.compareTo.
  • Consolidate the number of places where we update apiLocalCompleteTimeNanos
    and responseCompleteTimeNanos.
  • Added toString to ByteBufferSendandMultiRecordsSend`.
  • Restrict access to methods in QuotaTestClients to expose only what we need
    to.

Committer Checklist (excluded from commit message)

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

@ijuma ijuma requested a review from junrao April 28, 2020 17:49
@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Apr 28, 2020

2 jobs passed, 1 unrelated flaky test failed:

org.apache.kafka.streams.integration.QueryableStateIntegrationTest.shouldAllowConcurrentAccesses

junrao
junrao approved these changes Apr 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@ijuma : Thanks for the PR. LGTM. Just.a couple of minor comments below.

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

private def maybeRecordAndGetThrottleTimeMs(request: RequestChannel.Request): Int = {
val throttleTimeMs = quotas.request.maybeRecordAndGetThrottleTimeMs(request, time.milliseconds())
println(s"api throttle ms $throttleTimeMs ${request.header} ${request.header.clientId}")
Copy link
Copy Markdown
Contributor

@junrao junrao Apr 29, 2020

Choose a reason for hiding this comment

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

I guess this is not intended?

Copy link
Copy Markdown
Member Author

@ijuma ijuma Apr 29, 2020

Choose a reason for hiding this comment

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

Yes, sorry, forgot to remove.

Comment thread core/src/test/scala/integration/kafka/api/BaseQuotaTest.scala Outdated
}

def verifyProduceThrottle(expectThrottle: Boolean, verifyClientMetric: Boolean = true): Unit = {
def verifyThrottleTimeRequestChannelMetric(apiKey: ApiKeys, metricNameSuffix: String,
Copy link
Copy Markdown
Contributor

@junrao junrao Apr 29, 2020

Choose a reason for hiding this comment

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

Could this be private?

Copy link
Copy Markdown
Member Author

@ijuma ijuma Apr 29, 2020

Choose a reason for hiding this comment

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

Yes. I also restricted access to a few other methods in this class where possible.

@ijuma ijuma force-pushed the kafka-9652-request-throttle-fix branch from 71bb1e1 to a73ee74 Compare April 29, 2020 23:43
@ijuma ijuma merged commit 322b109 into apache:trunk Apr 30, 2020
@ijuma ijuma deleted the kafka-9652-request-throttle-fix branch April 30, 2020 03:09
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 30, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into `confluentinc…
31c13ff 
…/master`

* apache-github/trunk: (45 commits)
  MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587)
  KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567)
  KAFKA-9922: Update demo instructions in examples README (apache#8559)
  KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442)
  KAFKA-9875: Make integration tests more resilient (apache#8578)
  KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582)
  KAFKA-9925: decorate pseudo-topics with app id (apache#8574)
  KAFKA-9832: fix attempt to commit non-running tasks (apache#8580)
  KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540)
  MINOR: add support for kafka 2.4 and 2.5 to downgrade test
  KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568)
  KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548)
  KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541)
  MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572)
  KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564)
  Minor: remove redundant check in auto preferred leader election (apache#8566)
  MINOR: Update the link to the Raft paper in docs (apache#8560)
  MINOR: Fix typos in config properties in MM2 test (apache#8561)
  MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982)
  MINOR: document how to escape json parameters to ducktape tests (apache#8546)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 30, 2020
@ijuma
CONFLUENT: Sync from apache/kafka (29 April 2020)
1f84e45 
There was a minor conflict in gradle.properties because the default
Scala version changed upstream to Scala 2.13. I kept the upstream
change.

Related to this, I have updated Jenkinsfile to compile and validate
with Scala 2.12 in a separate stage so that we ensure we maintain
compatibility. Unlike Apache Kafka, we only run the tests with the
default Scala version, which is now 2.13.

* apache-github/trunk: (45 commits)
MINOR: Fix broken JMX link in docs by adding missing starting double
quote (apache#8587)
KAFKA-9652: Fix throttle metric in RequestChannel and request log due
to KIP-219 (apache#8567)
  KAFKA-9922: Update demo instructions in examples README (apache#8559)
KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses
(apache#8442)
  KAFKA-9875: Make integration tests more resilient (apache#8578)
KAFKA-9932: Don't load configs from ZK when the log has already been
loaded (apache#8582)
  KAFKA-9925: decorate pseudo-topics with app id (apache#8574)
  KAFKA-9832: fix attempt to commit non-running tasks (apache#8580)
KAFKA-9127: don't create StreamThreads for global-only topology
(apache#8540)
  MINOR: add support for kafka 2.4 and 2.5 to downgrade test
  KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568)
KAFKA-9823: Follow-up, check state for handling commit error response
(apache#8548)
  KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541)
MINOR: Fix partition numbering from 0 to P-1 instead of P in docs
(apache#8572)
KAFKA-9921: disable caching on stores configured to retain duplicates
(apache#8564)
Minor: remove redundant check in auto preferred leader election
(apache#8566)
  MINOR: Update the link to the Raft paper in docs (apache#8560)
  MINOR: Fix typos in config properties in MM2 test (apache#8561)
MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters.
(apache#7982)
MINOR: document how to escape json parameters to ducktape tests
(apache#8546)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@junrao junrao junrao approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ijuma @junrao