Skip to content

Conversation

@masteryhx
Copy link
Contributor

What is the purpose of the change

Support to read/write checkpoint metadata of merged files

Brief change log

  • serialize and deserialize SegmentFileStateHandle in MetadataV2V3SerializerBase
  • hotfix error equals for SegmentFileStateHandle

Verifying this change

This change added tests and can be verified as follows:

  • Add testSerializeSegmentStateHandle

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 11, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@masteryhx
Copy link
Contributor Author

@Zakelly @fredia Could you take a look ?

@Zakelly
Copy link
Contributor

Zakelly commented Mar 15, 2024

@Zakelly @fredia Could you take a look ?

@masteryhx
How about waiting for the FLINK-34668, where some new state handles will be introduced.

@masteryhx
Copy link
Contributor Author

@Zakelly @fredia Could you take a look ?

@masteryhx How about waiting for the FLINK-34668, where some new state handles will be introduced.

Thanks for the suggestion.
I just rework it after rebasing FLINK-34668 .

Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! Minor question:

dos.writeLong(relativeFileStateHandle.getStateSize());
} else if (stateHandle instanceof SegmentFileStateHandle) {
if (stateHandle instanceof EmptySegmentFileStateHandle) {
dos.writeByte(EMPTY_SEGMENT_FILE_HANDLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, there is not a chance that EmptySegmentFileStateHandle is serialized here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This serializeStreamStateHandle is used when serialize EmptyFileMergingOperatorStreamStateHandle which holds this as its delegated empty stream state handle.
So it's still needed, right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.... I missed the serializeStreamStateHandle(stateHandle.getDelegateStateHandle(), dos); part

Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

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

Thanks for the update! LGTM+1

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.

3 participants