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 `Optional[SnapshotReason]` we could use
`Set[SnapshotReason]`. Depending on the layer we can associate different means
to this set. When returned from `def shouldSnapshot()` the caller can assume
that if the set is empty it means that it should not generate a snapshot.
> 2. I would also need to change the function signature of
`createNewSnapshot()` in `KafkaMetadataLog` to accommodate the new
`SnapshotReason` parameter. Below is the new signature. Would this be okay?
> ```java
> Optional<RawSnapshotWriter> createNewSnapshot(OffsetAndEpoch snapshotId,
SnapshotReason reason);
> ```
How about:
```java
Optional<RawSnapshotWriter> createNewSnapshot(OffsetAndEpoch snapshotId,
Set<SnapshotReason> reasons);
```
If the set `reasons` is empty then the implementation of `createNewSnapshot`
can assume that the reason is unknown.
What do you think?
--
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
