[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1614319820 @jeqo , thanks for raising it. Yes, I forgot to pass the `bootstart.server` as `commonClientConfigs`, so that it won't be passed into the producer/consumer. Are you available to file a small PR to fix it? I can work on that next week if you are busy. 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
[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1594624985 Failed tests are unrelated and the failed `testReplication` also fail in trunk build. I've identified it's caused by this change: https://github.com/apache/kafka/pull/13838 . Go ahead to merge it. ``` Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders() Build / JDK 17 and Scala 2.13 / kafka.admin.AddPartitionsTest.testIncrementPartitions(String).quorum=zk Build / JDK 17 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders() Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldAllowRemovingAndAddingNamedTopologyToRunningApplicationWithMultipleNodesAndResetsOffsets() Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testReplication() ``` -- 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
[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1594043903 Still cannot get a healthy CI build results. Try to rebase to the latest trunk. -- 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
[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1591076070 > > I think we don't have this implemented. We should pass remote.log.metadata.* into RLMM based on KIP-405. Created [KAFKA-15083](https://issues.apache.org/jira/browse/KAFKA-15083) for this issue. > > @showuon This is no more valid, KIP needs to be updated with the prefix based configs for RSM and RLMM. Will update the KIP with those details. Good to know, thanks Satish. I've assigned KAFKA-15083 to you. You can close it once you've updated the KIP. -- 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
[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1588961245 @divijvaidya @satishd , PR updated. Thanks. > 1. We probably want to update the KIP-405 here: https://cwiki.apache.org/confluence/display/KAFKA/KIP-405%3A+Kafka+Tiered+Storage#KIP405:KafkaTieredStorage-Configs and specify that this config is optional Updated. > 2. From the javadoc of RLMM > When this is configured all other > required properties can be passed as properties with prefix of 'remote.log.metadata.manager.listener > Can we please add a test to verify this? (asking because while constructing the rlmmProps, we don't pass any other configs with the listener prefix) I think we don't have this implemented. We should pass `remote.log.metadata.*` into RLMM based on KIP-405. Created [KAFKA-15083](https://issues.apache.org/jira/browse/KAFKA-15083) for this issue. 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
[GitHub] [kafka] showuon commented on pull request #13828: KAFKA-15066: add "remote.log.metadata.manager.listener.name" config to rlmm
showuon commented on PR #13828: URL: https://github.com/apache/kafka/pull/13828#issuecomment-1586936366 > One last question Luke, should a unit test have failed when cluster.id property was missing in first revision? It probably hints towards a gap in testing. Can we add that? (or it's ok to say it will be added when Satish adds rest of the tests) The test is added in the last commit: https://github.com/apache/kafka/pull/13828/commits/39de62c32929a64d515912f2d4f219866921c705 Also, I reverted the `Endpoint` back to `Optional` because I re-read the javadoc of `RemoteLogMetadataManager`: https://github.com/apache/kafka/blob/513e1c641d63c5e15144f9fcdafa1b56c5e5ba09/storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogMetadataManager.java#L45-L48 It's saying this is not a required config. Only used when `needed by RemoteLogMetadataManager implementation`. We should not throw exception if not provided. -- 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
