Skip to content

Pipe: Handle hybrid meta progress indexes - #18331

Open
Caideyipi wants to merge 4 commits into
apache:masterfrom
Caideyipi:fix/pipe-hybrid-meta-progress-index
Open

Pipe: Handle hybrid meta progress indexes#18331
Caideyipi wants to merge 4 commits into
apache:masterfrom
Caideyipi:fix/pipe-hybrid-meta-progress-index

Conversation

@Caideyipi

Copy link
Copy Markdown
Collaborator

Description

Problem

IoTDBNonDataRegionSource assumed every non-minimum task progress was a MetaProgressIndex. Pipe task progress can also be merged into a HybridProgressIndex, and may be wrapped by a StateProgressIndex. This caused a ClassCastException while restoring a non-data-region source or calculating its remaining events during DataNode shutdown.

Changes

  • Extract the meta progress component from direct, hybrid, and state-wrapped progress indexes.
  • Use that component for source recovery and remaining-event accounting.
  • Conservatively fall back to snapshot recovery or full queue size when no meta component exists.
  • Add unit coverage for hybrid, state-wrapped hybrid, and hybrid-without-meta cases.

Verification

  • mvn spotless:apply -pl iotdb-core/node-commons
  • mvn test -pl iotdb-core/node-commons -Dtest=IoTDBNonDataRegionSourceTest

This PR has:

  • been self-reviewed.
  • added unit tests to cover new code paths.

Key changed/added classes
  • IoTDBNonDataRegionSource
  • IoTDBNonDataRegionSourceTest

…com/Caideyipi/iotdb into fix/pipe-hybrid-meta-progress-index

# Conflicts:
#	iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/source/IoTDBNonDataRegionSource.java
Comment thread iotdb-client/client-go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment on lines +168 to +199
public final <T extends ProgressIndex> Optional<T> getProgressIndexByType(
final Class<T> progressIndexClass) {
if (progressIndexClass.isInstance(this)) {
return Optional.of(progressIndexClass.cast(this));
}

if (this instanceof StateProgressIndex) {
return ((StateProgressIndex) this)
.getInnerProgressIndex()
.getProgressIndexByType(progressIndexClass);
}

if (this instanceof HybridProgressIndex) {
final Map<Short, ProgressIndex> type2Index = ((HybridProgressIndex) this).getType2Index();

// Prefer a direct component over one nested in another composite progress index.
for (final ProgressIndex progressIndex : type2Index.values()) {
if (progressIndexClass.isInstance(progressIndex)) {
return Optional.of(progressIndexClass.cast(progressIndex));
}
}
for (final ProgressIndex progressIndex : type2Index.values()) {
final Optional<T> extractedProgressIndex =
progressIndex.getProgressIndexByType(progressIndexClass);
if (extractedProgressIndex.isPresent()) {
return extractedProgressIndex;
}
}
}

return Optional.empty();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let the sub-classes implement it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants