[GitHub] [kafka] jsancio commented on pull request #12414: KAFKA-14073 Logging the reason for Snapshot

2022-09-07 Thread GitBox
jsancio commented on PR #12414: URL: https://github.com/apache/kafka/pull/12414#issuecomment-1239588238 > Hi @jsancio could you please review this PR? Sorry for the multiple changes this has required. Thanks for the changes and excuse the delays. LGTM in general. Restarted the build

[GitHub] [kafka] jsancio commented on pull request #12414: KAFKA-14073 Logging the reason for Snapshot

2022-08-31 Thread GitBox
jsancio commented on PR #12414: URL: https://github.com/apache/kafka/pull/12414#issuecomment-1233255286 > Hi @jsancio, I have reverted the changes made to `RaftClient` and kept the logging withing `BrokerMetadataSnapshotter` and `QuorumController` > > I have two questions, would

[GitHub] [kafka] jsancio commented on pull request #12414: KAFKA-14073 Logging the reason for Snapshot

2022-08-05 Thread GitBox
jsancio commented on PR #12414: URL: https://github.com/apache/kafka/pull/12414#issuecomment-1206916291 > Hi @jsancio, just one another doubt - Does this mean we don't want to log the reason within `KafkaMetadataLog` and keep the logging as it is right now within

[GitHub] [kafka] jsancio commented on pull request #12414: KAFKA-14073 Logging the reason for Snapshot

2022-08-05 Thread GitBox
jsancio commented on PR #12414: URL: https://github.com/apache/kafka/pull/12414#issuecomment-1206405757 > Thanks @jsancio, this is helpful. I'll go ahead and make these changes! @ashmeet13 , I thought about this some more and I don't think we should include the reason for the

[GitHub] [kafka] jsancio commented on pull request #12414: KAFKA-14073 Logging the reason for Snapshot

2022-08-04 Thread GitBox
jsancio commented on PR #12414: URL: https://github.com/apache/kafka/pull/12414#issuecomment-1205508037 Thanks for the changes @ashmeet13 they look good in general. > 1. How do we handle multiple reasons for starting a snapshot in an enum? Instead of using