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
