MINOR: Proactively update producer topic access time.#7672
Conversation
There was a problem hiding this comment.
I don't think it's safe to reuse nowMs here since we may have blocked on free.allocate.
There was a problem hiding this comment.
Done. Common case should be that there's already an in-progress batch above, so shouldn't be a big deal to always call time.milliseconds() here even if there's no memory pressure.
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the PR. This looks good generally, nice to see some unnecessary milliseconds() calls being removed. As noted in a separate comment, I think we have to get a fresh time after allocation since that can block if the buffer is full. @rajinisivaram, since you implemented the metadata expiry a while back, do the changes here look good to you?
rajinisivaram
left a comment
There was a problem hiding this comment.
@bdbyrne Thanks for the PR. As @ijuma has said, we need to update time after the potentially blocking buffer allocation, but the rest of the changes look good.
We currently expire topics that are not being actively used. Topics are expired 5 minutes after the metadata response following the last time the topic was used. With the changes in this PR, we continue to expire unused topics, but with slight difference that we are expiring 5 minutes after the last use. I think the main reason for the sentinel value was to avoid getting time just to set expiry, while with your changes since the value is available anyway, it makes sense to use it. A slight difference now is that we rely on successful metadata response to be received within the hard-coded 5 minutes. Just wanted to confirm that my understanding is correct and we are expecting this to be a reasonable limit.
|
@bdbyrne Thanks for the PR, LGTM. Can we clarify the relationship between this PR and the issue described in https://issues.apache.org/jira/browse/KAFKA-9164? The JIRA says: We were updating the access time for the topic whenever a record was sent. And that seems to be the case even with this PR. So it is not obvious to me what the issue was and how this PR addresses the issue. |
You're correct, I was mistaken. I've closed the issue and updated the description of this PR. |
|
retest this please |
rajinisivaram
left a comment
There was a problem hiding this comment.
@bdbyrne Thanks for the PR, LGTM. Rerunning tests since the results from the previous run are not available for checking, will merge after that completes.
|
Test failure not related, SaslScramSslEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl is a known flaky test with a PR open to address it. Merging to trunk. |
Changes the ProducerMetadata to longer record a sentinel TOPIC_EXPIRY_NEEDS_UPDATE upon topic map emplacement, and instead set the expiry time directly. Previously the expiry time was being updated for all touched topics after a metadata fetch was processed, which could be seconds/minutes in the future.
Additionally propagates the current time further in the Producer, which should reduce the total number of current-time calls.
Committer Checklist (excluded from commit message)