Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] Support restore/rollback sync during conversion (1/2) #569

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e0a8710
Add a source identifier in target table transaction
danielhumanmod Oct 27, 2024
9bc8256
detect rollback from source and target table
danielhumanmod Oct 27, 2024
76e0bc0
boundary safe
danielhumanmod Oct 28, 2024
45b279a
fix existing tests
danielhumanmod Oct 28, 2024
8ac5717
support commit level info
danielhumanmod Nov 22, 2024
53a58b6
remove redundant changes
danielhumanmod Nov 22, 2024
bb5ed78
simplify source id generation
danielhumanmod Nov 22, 2024
9b8b71e
remove rollback detection in this PR
danielhumanmod Nov 22, 2024
252ca2c
format
danielhumanmod Nov 22, 2024
d9cb5fd
avoid hard code
danielhumanmod Nov 22, 2024
c8dde9e
format
danielhumanmod Nov 22, 2024
bfbb78f
use timestamp as source identifier for Hudi
danielhumanmod Nov 22, 2024
002d91b
optimize early termination logic
danielhumanmod Nov 24, 2024
bf5f0a0
source-target identifier mapping test
danielhumanmod Nov 24, 2024
56fe85e
introduce source metadata
danielhumanmod Dec 4, 2024
a40cd6c
make some methods private
danielhumanmod Dec 11, 2024
1e39d1a
make xtable metadata commit level in delta and iceberg
danielhumanmod Dec 13, 2024
0af0a82
simplify hudi metadata operation
danielhumanmod Dec 13, 2024
82b2d2d
refactor get target commit process
danielhumanmod Dec 13, 2024
7ffda25
format
danielhumanmod Dec 13, 2024
4198e5b
improve doc and test
danielhumanmod Dec 14, 2024
b181a07
format
danielhumanmod Dec 14, 2024
4ca9c9a
Merge branch 'main' of https://github.com/danielhumanmod/incubator-xt…
danielhumanmod Jan 23, 2025
9552679
fix compile error
danielhumanmod Jan 23, 2025
5fe489e
make sourceIdentifier nonnull without default value
danielhumanmod Jan 25, 2025
42c804b
keep existing table-level metadata
danielhumanmod Jan 25, 2025
e470e98
standard way for doc
danielhumanmod Jan 25, 2025
2033c3a
complete tests after making sourceIdentifier nonnull
danielhumanmod Jan 25, 2025
edb99fd
using TimelineUtils#getCommitMetadata to extract commit metadata
danielhumanmod Jan 25, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import lombok.Builder;
import lombok.Value;

import org.apache.xtable.model.metadata.SourceMetadata;
import org.apache.xtable.model.storage.PartitionFileGroup;

/**
Expand All @@ -47,4 +48,6 @@ public class InternalSnapshot {
List<PartitionFileGroup> partitionedDataFiles;
// pending commits before latest commit on the table.
@Builder.Default List<Instant> pendingCommits = Collections.emptyList();
// Metadata about source table
SourceMetadata sourceMetadata;
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import lombok.Builder;
import lombok.Value;

import org.apache.xtable.model.metadata.SourceMetadata;
import org.apache.xtable.model.storage.DataFilesDiff;

/**
Expand All @@ -36,4 +37,6 @@ public class TableChange {

/** The {@link InternalTable} at the commit time to which this table change belongs. */
InternalTable tableAsOfChange;

SourceMetadata sourceMetadata;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.xtable.model.metadata;

import java.io.IOException;

import lombok.Builder;
import lombok.Value;

import com.fasterxml.jackson.databind.ObjectMapper;

import org.apache.xtable.model.exception.ParseException;

@Value
@Builder
public class SourceMetadata {
// The source table snapshot identifier
// Snapshot ID in Iceberg, version ID in Delta, and instant <timestamp_action> in Hudi
String sourceIdentifier;
// The table format of the source table
String tableFormat;

private static final ObjectMapper MAPPER = new ObjectMapper();

public static SourceMetadata fromJson(String json) throws ParseException {
try {
return MAPPER.readValue(json, SourceMetadata.class);
} catch (IOException e) {
throw new ParseException("Failed to deserialize SourceMetadata", e);
}
}

public String toJson() {
try {
return MAPPER.writeValueAsString(this);
} catch (IOException e) {
throw new ParseException("Failed to serialize SourceMetadata", e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public class TableSyncMetadata {
/** Property name for the XTABLE metadata in the table metadata/properties */
public static final String XTABLE_METADATA = "XTABLE_METADATA";

public static final String XTABLE_SOURCE_METADATA = "XTABLE_SOURCE_METADATA";
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a separate metadata, is it possible to embed this within the metadata object that we have today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! The current metadata is table-level information, while the new one is commit-level, but it is true that we can reuse this key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And maybe we could consider moving the source format to table-level information and leaving the source identifier at the commit level to reduce redundancy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if all of the information should actually be stored at the commit level instead of table level. In the Hudi target, the information is already stored at the commit level since that was the intention and the format I knew best. The others, it seems like I did not add the metadata to the correct location.

The source format should be allowed to change over time for flexibility so I think it is best to keep that at the commit level as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see your point, that's a good way. I’ll get started on it.


Instant lastInstantSynced;
List<Instant> instantsToConsiderForNextSync;
int version;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,13 @@ public interface ConversionSource<COMMIT> extends Closeable {
* false.
*/
boolean isIncrementalSyncSafeFrom(Instant instant);

/**
* Extract the identifier of the provided commit, the identifier defined as 1. Snapshot ID in
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use <ul><li> tags for formatting the docs with the list if you want

* Iceberg 2. Version ID in Delta 3. timestamp in Hudi
*
* @param commit The provided commit
* @return the string version of commit identifier
*/
String getCommitIdentifier(COMMIT commit);
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import org.apache.xtable.conversion.TargetTable;
import org.apache.xtable.model.InternalTable;
import org.apache.xtable.model.metadata.SourceMetadata;
import org.apache.xtable.model.metadata.TableSyncMetadata;
import org.apache.xtable.model.schema.InternalPartitionField;
import org.apache.xtable.model.schema.InternalSchema;
Expand Down Expand Up @@ -76,8 +77,9 @@ public interface ConversionTarget {
* Starts the sync and performs any initialization required
*
* @param table the table that will be synced
* @param sourceMetadata The source table metadata for current round of sync
*/
void beginSync(InternalTable table);
void beginSync(InternalTable table, SourceMetadata sourceMetadata);

/** Completes the sync and performs any cleanup required. */
void completeSync();
Expand All @@ -90,4 +92,7 @@ public interface ConversionTarget {

/** Initializes the client with provided configuration */
void init(TargetTable targetTable, Configuration configuration);

/** Return the commit identifier from target table */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you be more specific here? I am assuming this is the latest commit identifier but it should be more specific if the table can have more than one over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you be more specific here? I am assuming this is the latest commit identifier but it should be more specific if the table can have more than one over time.

Do you mean the snapshot sync scenario? Because In incremental sync, we will ensure target and source commit has 1:1 mapping. However, in snapshot sync, we will use the commit id of current (latest) snapshot even though the changes might contains more than one commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To ensure we’re aligned, the context of this function is:

It is designed to retrieve the corresponding target commit based on a given source identifier during a sync operation. Here’s the scenario:

Assume:

  • Last completed sync (1 snapshot, 1 incremental) is source commit 3
  • We’re starting the new sync process from source commit 4

Source table commit history:
• 1 (UPDATE)
• 2 (UPDATE)
• 3 (UPDATE)
• 4 (ROLLBACK to 2)
• 5 (UPDATE)
• 6 (UPDATE)
Target table commit history (mapped by source identifiers):
• 1 (mapped to source id 2)
• 2 (mapped to source id 3)

When syncing the ROLLBACK operation (source commit 4), we need to identify the corresponding target commit that aligns with the source identifier 2 (which is 1 in target).

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me but the java doc can be a bit more clear. It should also include context on when the Optional will be empty

Optional<String> getTargetCommitIdentifier(String sourceIdentifier);
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.apache.xtable.model.InternalSnapshot;
import org.apache.xtable.model.InternalTable;
import org.apache.xtable.model.TableChange;
import org.apache.xtable.model.metadata.SourceMetadata;
import org.apache.xtable.model.metadata.TableSyncMetadata;
import org.apache.xtable.model.sync.SyncMode;
import org.apache.xtable.model.sync.SyncResult;
Expand Down Expand Up @@ -73,7 +74,8 @@ public Map<String, SyncResult> syncSnapshot(
internalTable,
target -> target.syncFilesForSnapshot(snapshot.getPartitionedDataFiles()),
startTime,
snapshot.getPendingCommits()));
snapshot.getPendingCommits(),
snapshot.getSourceMetadata()));
} catch (Exception e) {
log.error("Failed to sync snapshot", e);
results.put(
Expand Down Expand Up @@ -121,7 +123,8 @@ public Map<String, List<SyncResult>> syncChanges(
change.getTableAsOfChange(),
target -> target.syncFilesForDiff(change.getFilesDiff()),
startTime,
changes.getPendingCommits()));
changes.getPendingCommits(),
change.getSourceMetadata()));
} catch (Exception e) {
log.error("Failed to sync table changes", e);
resultsForFormat.add(buildResultForError(SyncMode.INCREMENTAL, startTime, e));
Expand Down Expand Up @@ -149,9 +152,10 @@ private SyncResult getSyncResult(
InternalTable tableState,
SyncFiles fileSyncMethod,
Instant startTime,
List<Instant> pendingCommits) {
List<Instant> pendingCommits,
SourceMetadata sourceMetadata) {
// initialize the sync
conversionTarget.beginSync(tableState);
conversionTarget.beginSync(tableState, sourceMetadata);
// sync schema updates
conversionTarget.syncSchema(tableState.getReadSchema());
// sync partition updates
Expand Down
Loading
Loading