Skip to content

Conversation

@luoyuxia
Copy link
Contributor

@luoyuxia luoyuxia commented Jan 7, 2026

Purpose

Linked issue: close #xxx

Brief change log

Tests

API and Format

Documentation

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an issue with union read restoration by introducing a new serialization version (VERSION_1) for SourceEnumeratorState. The key change is that VERSION_1 always serializes and deserializes remainingHybridLakeFlussSplits regardless of the lakeSource flag, fixing a bug where VERSION_0 would only serialize/deserialize these splits when lakeSource was non-null.

Key Changes:

  • Introduced VERSION_1 serialization format that always handles remainingHybridLakeFlussSplits
  • Refactored serialization logic to extract common bucket/partition serialization into a helper method
  • Updated deserialization to handle both VERSION_0 (backward compatible) and VERSION_1 formats
  • Added comprehensive tests for V0 compatibility and cross-version scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
FlussSourceEnumeratorStateSerializer.java Introduces VERSION_1 serialization format, refactors serialization logic into helper methods, and updates deserialization to handle version-specific behavior
SourceEnumeratorStateSerializerTest.java Adds tests for V0 backward compatibility and cross-version deserialization scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@luoyuxia luoyuxia force-pushed the fix-union-read-fluss-state branch from 39ed2ad to c3bf44a Compare January 7, 2026 05:42
@luoyuxia luoyuxia force-pushed the fix-union-read-fluss-state branch from c3bf44a to ccacaaf Compare January 7, 2026 06:01
Copy link
Contributor

@loserwang1024 loserwang1024 left a comment

Choose a reason for hiding this comment

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

I have left some message.

import java.util.Set;

import static org.apache.fluss.utils.Preconditions.checkNotNull;

Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to add what's the different with different version? such as:
image

@Override
public SourceEnumeratorState deserialize(int version, byte[] serialized) throws IOException {
if (version != VERSION_0) {
if (version != VERSION_0 && version != CURRENT_VERSION) {
Copy link
Contributor

Choose a reason for hiding this comment

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

version != VERSION_0 && version != CURRENT_VERSION

When we support version 3 later, state with version 3 cannot downgrade. We have met this problem in many connectors. @leonardBang , CC, should we stop higher version's state?

And I also found a problem : we can never downgrade later to version 1. Because in later code will version 1 state: version != VERSION_0 is always true. Maybe we should add this to upgrade document later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1: so, what's the best pratice in here? Always make the version check pass and make sure version3 compitable with v2? From kafka connector code, seems the kafka connector will still have the problem?

2: what do you mean by saying we can never downgrade later to version 1? Seems it still the same problem to 1?

Copy link
Contributor

@loserwang1024 loserwang1024 Jan 8, 2026

Choose a reason for hiding this comment

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

I understand it. But in your follower comment "for VERSION_1 and later" maybe inconsistency.

image

Maybe you can use switch.. case as kafka does. It's clearer
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, that's clearer

@luoyuxia luoyuxia force-pushed the fix-union-read-fluss-state branch from 65dbbdd to d0fc306 Compare January 7, 2026 13:11
@luoyuxia
Copy link
Contributor Author

luoyuxia commented Jan 8, 2026

@loserwang1024 Comments is addressed. PTAL

Copy link
Contributor

@loserwang1024 loserwang1024 left a comment

Choose a reason for hiding this comment

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

LGTM

@luoyuxia luoyuxia merged commit 38c2555 into apache:main Jan 8, 2026
6 checks passed
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