AndrewJSchofield commented on code in PR #17573:
URL: https://github.com/apache/kafka/pull/17573#discussion_r1814864276
##########
share/src/main/java/org/apache/kafka/server/share/persister/PartitionFactory.java:
##########
@@ -26,7 +26,7 @@
*/
public class PartitionFactory {
public static final int DEFAULT_STATE_EPOCH = 0;
- public static final int DEFAULT_START_OFFSET = 0;
+ public static final int DEFAULT_START_OFFSET = -1;
Review Comment:
This is probably the uninitialized start offset, I would say. Negative
values have special meanings for offsets I think.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
@@ -238,4 +264,39 @@ public int shareHeartbeatIntervalMs() {
public int shareRecordLockDurationMs() {
return shareRecordLockDurationMs;
}
+
+ /**
+ * The share group auto offset reset strategy.
+ */
+ public ShareGroupAutoOffsetReset shareAutoOffsetReset() {
+ return ShareGroupAutoOffsetReset.forStrategy(shareAutoOffsetReset);
+ }
+
+ public enum ShareGroupAutoOffsetReset {
+ LATEST("latest"),
+ EARLIEST("earliest"),
+ UNKNOWN("unknown");
+
+ private final String strategy;
+
+ ShareGroupAutoOffsetReset(String strategy) {
Review Comment:
You certainly could use `OffsetResetStrategy` as a model for the enum. You
don't need to use the string like this. Look at ConsumerConfig.java, where it
uses `Utils.enumOptions` to initialize the value.
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -382,7 +387,35 @@ public CompletableFuture<Void> maybeInitialize() {
}
// Set the state epoch and end offset from the persisted state.
- startOffset = partitionData.startOffset() != -1 ?
partitionData.startOffset() : 0;
+ if (partitionData.startOffset() !=
PartitionFactory.DEFAULT_START_OFFSET) {
+ startOffset = partitionData.startOffset();
+ } else {
+ GroupConfig.ShareGroupAutoOffsetReset offsetResetStrategy;
+ if (groupConfigManager.groupConfig(groupId).isPresent()) {
+ offsetResetStrategy =
groupConfigManager.groupConfig(groupId).get().shareAutoOffsetReset();
+ if (offsetResetStrategy ==
GroupConfig.ShareGroupAutoOffsetReset.UNKNOWN) {
+ offsetResetStrategy =
GroupConfig.defaultShareAutoOffsetReset();
+ }
+ } else {
+ offsetResetStrategy =
GroupConfig.defaultShareAutoOffsetReset();
+ }
+
+ if (offsetResetStrategy ==
GroupConfig.ShareGroupAutoOffsetReset.EARLIEST) {
Review Comment:
I would tend to structure this as a surrounding try-catch, and then the
if-else inside. Only needs one catch block, and a bit less duplicated code.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
@@ -238,4 +264,39 @@ public int shareHeartbeatIntervalMs() {
public int shareRecordLockDurationMs() {
return shareRecordLockDurationMs;
}
+
+ /**
+ * The share group auto offset reset strategy.
+ */
+ public ShareGroupAutoOffsetReset shareAutoOffsetReset() {
+ return ShareGroupAutoOffsetReset.forStrategy(shareAutoOffsetReset);
+ }
+
+ public enum ShareGroupAutoOffsetReset {
Review Comment:
I would keep them separate so we can evolve them separately.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
