Skip to content

fix: support deserializing activeTasks/publishingTasks in KafkaSupervisorReportPayload - #20558

Open
cecemei wants to merge 4 commits into
apache:masterfrom
cecemei:task
Open

cecemei wants to merge 4 commits into
apache:masterfrom
cecemei:task

Conversation

@cecemei

@cecemei cecemei commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Description

Support deserializing activeTasks/publishingTasks in KafkaSupervisorReportPayload.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new deserialization path still erases Kafka-specific offset types: real active-task reports will deserialize nested offsets with untyped keys and values, so offset lookups and round-trip equality are incorrect. Preserve the Kafka generic types before merging.

Reviewed 5 of 5 changed files; findings in 1 file.

Validation: git diff --check bb50f29b1d792c7b82e48dd24435be5966e8b269..HEAD passed. No builds or tests were run.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

@JsonProperty("ignored2") SupervisorStateManager.State detailedState,
@JsonProperty("recentErrors") List<SupervisorStateManager.ExceptionEvent> recentErrors
@JsonProperty("recentErrors") List<SupervisorStateManager.ExceptionEvent> recentErrors,
@JsonProperty("activeTasks") @Nullable List<TaskReportData> activeTasks,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Raw task lists erase Kafka offset types

Finding: Because these creator parameters are raw List, Jackson constructs each task without the KafkaTopicPartition/Long type bindings. When a real status contains non-empty startingOffsets, currentOffsets, or lag maps, their keys are deserialized as strings and offset values may be untyped numeric values, so lookups by KafkaTopicPartition can return null or casts can fail even though the task list itself is present.

Suggestion: Preserve the concrete TaskReportData<KafkaTopicPartition, Long> types through the payload constructors and extend the round-trip test with non-null partition-offset and lag maps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants