zhang-arvin commented on PR #20092: URL: https://github.com/apache/druid/pull/20092#issuecomment-5886841467
Thanks @FrankChen021. Addressed in 0de811ff, one by one: **1. `SeekableStreamSupervisor.java:4468` Pre-reset task groups omit reset partitions — confirmed, fixed.** You are right, and removing the group from `newTaskGroups` (the previous revision) was not enough: `generateStartingSequencesForPartitionGroup` leaves a partition out entirely when `getOffsetFromStorageForPartition` adds it to `partitionsToReset`, so a rebuilt group would have had no offset for it. The construction of a `TaskGroup` is now extracted into `buildNewTaskGroup`, and after the batch reset the affected groups are rebuilt with it, in place, using the reset `partitionOffsets` — both the mixed group (reset partition included, reading from the reset position from the start) and the all-stale group (non-empty, and a task is created for it in the same run). **2. `KafkaIndexTaskRunner.java:85` Future offsets now fail instead of waiting — confirmed, fixed.** The re-throw is now conditional on the offset actually being *below* the earliest available offset. Everything else the exception reports is compared against the earliest and the latest offset of each partition: an offset above the latest one is legitimate future work, so the task waits and retries as the removed loop did, instead of failing until the log catches up. When at least one partition is below the earliest and `resetOffsetAutomatically` is set, the exception is still re-thrown so the supervisor sees the reset request (a mixed exception covering both cases also re-throws, since that is the safe direction). **3. `KafkaIndexTaskRunner.java:137` Deprecated `TypeFactory.constructParametrizedType` (CodeQL) — confirmed, fixed.** This call is in the removed `deserializePartitionsFromMetadata` body that this PR moved, so it is part of this diff. It now uses `constructParametricType(SeekableStreamEndSequenceNumbers.class, KafkaTopicPartition.class, Long.class)`; `mapper.convertValue` is still passed the target type explicitly, so deserialization behaviour is unchanged. Not done in this push: the CI failures are still from master drift rather than these changes, and the fork build is unavailable locally (druid's offline build is missing `software.amazon.awssdk:bom` / `cyclonedx-maven-plugin`, and a single-module build needs the upstream test-jars installed), so this is compile-checked statically and verified by CI. 🤖 Generated by an AI agent (zhang-arvin). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
