Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
chia7712 commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2016328832 ``` ./gradlew cleanTest :trogdor:test --tests CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated :connect:mirror:test --tests MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testCreatePartitionsUseProvidedForwardingAdmin :core:test --tests DelegationTokenEndToEndAuthorizationWithOwnerTest.testProduceConsumeTopicAutoCreateTopicCreateAcl --tests DelegationTokenEndToEndAuthorizationWithOwnerTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl --tests SocketServerTest.testControlPlaneTakePrecedenceOverInterBrokerListenerAsPrivilegedListener --tests SocketServerTest.testZeroMaxConnectionsPerIp --tests SocketServerTest.testStagedListenerShutdownWhenConnectionQueueIsFull --tests SocketServerTest.testStagedListenerStartup --tests SocketServerTest.testControlPlaneAsPrivilegedListener --tests SocketServerTest.testInterBrokerListenerAsPrivilegedListener --tests DynamicBrokerReconfigurationTest.testThreadPoolResize --tests ReplicaManagerTest.testRemoteFetchExpiresPerSecMetric --tests SaslPlainPlaintextConsumerTest.testClusterResourceListener --tests TransactionsTest.testFencingOnSend --tests TransactionsTest.testCommitTransactionTimeout --tests LogDirFailureTest.testIOExceptionDuringLogRoll --tests LogDirFailureTest.testIOExceptionDuringCheckpoint :clients:test --tests SelectorTest.testConnectionsByClientMetric --tests Tls12SelectorTest.testConnectionsByClientMetric --tests SelectorTest.testInboundConnectionsCountInConnectionCreationMetric --tests Tls12SelectorTest.testInboundConnectionsCountInConnectionCreationMetric --tests SelectorTest.testMuteOnOOM --tests Tls12SelectorTest.testMuteOnOOM ``` the failed tests are unrelated and they pass on my local -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
johnnychhsu commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2015136213 @vamossagar12 thanks for the reminder. Updated the PR description to reflect the latest changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
vamossagar12 commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2014566088 @johnnychhsu , you could probably update the PR description to reflect the latest changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
johnnychhsu commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2012073628 thanks @vamossagar12 @ijuma for the comments. That's a fair point, I agree that a lock is heavy, and `volatile` should be good enough for this. Let me change this, thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
ijuma commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2011383134 I agree that the current behavior doesn't look right, but adding a lock is also too heavyweight. `volatile` seems reasonable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
vamossagar12 commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2009204011 > So you mean only if the config(MetricConfig config) works, then we need to return the updated config, right? yes, that's correct. Any other thread should see the updated state of the shared variables in the synchronised block only after the lock is released. And come to think of it, that is already being achieved without the synchronised block in the `config()` method. The changes you have added, provide the same guarantees as whatever exist today but at the expense of adding a lock. IF we want to make the value of the `config` object visible immediately to other threads, we could consider making it `volatile` but i am not sure if we really need it. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
johnnychhsu commented on PR #15550: URL: https://github.com/apache/kafka/pull/15550#issuecomment-2007003985 Thanks for the review @vamossagar12 . let me rephrase your comment to make sure that I understand you correctly. > Now, if some other thread is trying to update the config by using config(MetricConfig config), then unless that operation > doesn't go through, we can keep returning the unchanged config object. So you mean only if the `config(MetricConfig config)` works, then we need to return the updated config, right? I feel it's better to have it here, because if someone update the config, then immediately use the config as a criteria to do something else, but the config has not updated yet or somehow fails, then this could lead to a potential problem. wdyt? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16381: add lock for KafkaMetric config getter [kafka]
johnnychhsu opened a new pull request, #15550:
URL: https://github.com/apache/kafka/pull/15550
## Context
In KafkaMetirc.java, the config getter is
```
@Override
public MetricName metricName()
{ return this.metricName; }
```
and there is a setter
```
public void config(MetricConfig config) {
synchronized (lock) {
this.config = config;
}
}
```
Since it's possible to set and get in the mean time, we should have lock in
the getter as well
Refer to [KAFKA-16381](https://issues.apache.org/jira/browse/KAFKA-16381).
## Solution
Add the lock in the config getter
## Test
```
./gradlew cleanTest core:test --tests ConnectionQuotasTest --tests
ControllerMutationQuotaTest stream:test --tests StandbyTaskTest --tests
StreamTaskTest
```
and it passed
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
