-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(client): enrich write commit callback message and fire it for table-service commits #18988
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
Changes from all commits
c6d042b
31f6990
d426fb7
c2d0500
48d3528
e05c9a0
6c5d2d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,20 +19,26 @@ | |
|
|
||
| import org.apache.hudi.ApiMaturityLevel; | ||
| import org.apache.hudi.PublicAPIClass; | ||
| import org.apache.hudi.callback.HoodieWriteCommitCallbackUtil; | ||
| import org.apache.hudi.common.model.HoodieWriteStat; | ||
| import org.apache.hudi.common.table.view.TableFileSystemView.BaseFileOnlyView; | ||
| import org.apache.hudi.common.util.Lazy; | ||
| import org.apache.hudi.common.util.Option; | ||
|
|
||
| import lombok.AllArgsConstructor; | ||
| import lombok.AccessLevel; | ||
| import lombok.Getter; | ||
|
|
||
| import java.io.IOException; | ||
| import java.io.ObjectOutputStream; | ||
| import java.io.Serializable; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
|
|
||
| /** | ||
| * Base callback message, which contains commitTime and tableName only for now. | ||
| */ | ||
| @AllArgsConstructor | ||
| @Getter | ||
| @PublicAPIClass(maturity = ApiMaturityLevel.EVOLVING) | ||
| public class HoodieWriteCommitCallbackMessage implements Serializable { | ||
|
|
@@ -69,10 +75,116 @@ public class HoodieWriteCommitCallbackMessage implements Serializable { | |
| */ | ||
| private final Option<Map<String, String>> extraMetadata; | ||
|
|
||
| /** | ||
| * Previous base file paths keyed by fileId, derived from {@link #hoodieWriteStat} and the | ||
| * {@link BaseFileOnlyView} handed over by the write client, so that callback | ||
| * implementations don't have to rebuild a view themselves. Empty for inserts and for | ||
| * callers that don't supply a view. | ||
| * | ||
| * <p>Holds the resolved map once {@link #getPrevFilePaths()} has run, and stays null until | ||
| * then. Not transient: this is the copy that crosses Java serialization, which is why | ||
| * {@link #writeObject} forces resolution before writing. Excluded from the generated | ||
| * getters so it is published only through {@link #getPrevFilePaths()}. | ||
| */ | ||
| @Getter(AccessLevel.NONE) | ||
| private volatile Map<String, PrevFilePaths> prevFilePaths; | ||
|
|
||
| /** | ||
| * Resolves {@link #prevFilePaths} on demand. Resolution is deferred until the first | ||
| * {@link #getPrevFilePaths()} call, so a callback that never reads the previous paths pays | ||
| * nothing (no FileSystemView access at all). Transient because it captures a | ||
| * FileSystemView supplier, which is not serializable: on a deserialized instance this is | ||
| * null and the already-resolved {@link #prevFilePaths} is used instead. Excluded from the | ||
| * generated getters so the {@link Lazy} wrapper never leaks into JSON. | ||
| */ | ||
| @Getter(AccessLevel.NONE) | ||
| private final transient Lazy<Map<String, PrevFilePaths>> prevFilePathsResolver; | ||
|
|
||
| /** | ||
| * Free-form context that producers can attach for downstream callback consumers. | ||
| * The OSS write client populates this as empty; specialized callsites or wrappers | ||
| * may populate it with whatever context their callbacks need. | ||
| */ | ||
| private final Map<String, String> extraContext; | ||
|
|
||
| public HoodieWriteCommitCallbackMessage(String commitTime, | ||
| String tableName, | ||
|
codope marked this conversation as resolved.
|
||
| String basePath, | ||
| List<HoodieWriteStat> hoodieWriteStat, | ||
| Option<String> commitActionType, | ||
| Option<Map<String, String>> extraMetadata, | ||
| Supplier<BaseFileOnlyView> fsViewSupplier, | ||
| Map<String, String> extraContext) { | ||
| this.commitTime = commitTime; | ||
| this.tableName = tableName; | ||
| this.basePath = basePath; | ||
| this.hoodieWriteStat = hoodieWriteStat; | ||
| this.commitActionType = commitActionType; | ||
| this.extraMetadata = extraMetadata; | ||
| this.prevFilePathsResolver = Lazy.lazily(() -> HoodieWriteCommitCallbackUtil.resolvePrevFilePaths( | ||
| hoodieWriteStat, fsViewSupplier == null ? null : fsViewSupplier.get())); | ||
| this.extraContext = extraContext; | ||
| } | ||
|
|
||
| public HoodieWriteCommitCallbackMessage(String commitTime, | ||
| String tableName, | ||
| String basePath, | ||
| List<HoodieWriteStat> hoodieWriteStat) { | ||
| this(commitTime, tableName, basePath, hoodieWriteStat, Option.empty(), Option.empty()); | ||
| this(commitTime, tableName, basePath, hoodieWriteStat, Option.empty(), Option.empty(), | ||
| null, Collections.emptyMap()); | ||
| } | ||
|
|
||
| public HoodieWriteCommitCallbackMessage(String commitTime, | ||
| String tableName, | ||
| String basePath, | ||
| List<HoodieWriteStat> hoodieWriteStat, | ||
| Option<String> commitActionType, | ||
| Option<Map<String, String>> extraMetadata) { | ||
| this(commitTime, tableName, basePath, hoodieWriteStat, commitActionType, extraMetadata, | ||
| null, Collections.emptyMap()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the previous base file paths keyed by fileId, resolving them from the file-system | ||
| * view on first access and memoizing the result. A consumer that never calls this triggers | ||
| * no FileSystemView lookup. Never null: empty when no view was supplied and when the commit | ||
| * only inserted. | ||
| */ | ||
| public Map<String, PrevFilePaths> getPrevFilePaths() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry sagar, I asked Codex and it seems the Ser/De customization is unavoidable: The PR description already claims this behavior, but the current implementation does not provide it: `prevFilePaths` is a `transient Lazy`, and the round-trip test explicitly expects the paths to disappear.
Use two fields:
- A normal serializable field containing the resolved map.
- A transient lazy resolver containing the non-serializable filesystem-view supplier.
Then force resolution only when Java serialization actually occurs.
```java
import java.io.IOException;
import java.io.ObjectOutputStream;
@Getter(AccessLevel.NONE)
private volatile Map<String, PrevFilePaths> prevFilePaths;
@Getter(AccessLevel.NONE)
private final transient Lazy<Map<String, PrevFilePaths>> prevFilePathsResolver;Initialize them in the constructor: this.prevFilePaths = null;
this.prevFilePathsResolver = Lazy.lazily(() ->
HoodieWriteCommitCallbackUtil.resolvePrevFilePaths(
hoodieWriteStat,
fsViewSupplier == null ? null : fsViewSupplier.get()));The getter memoizes the resolved value: public Map<String, PrevFilePaths> getPrevFilePaths() {
Map<String, PrevFilePaths> paths = prevFilePaths;
if (paths == null) {
Lazy<Map<String, PrevFilePaths>> resolver = prevFilePathsResolver;
paths = resolver == null
? Collections.emptyMap()
: resolver.get();
prevFilePaths = paths == null
? Collections.emptyMap()
: paths;
}
return prevFilePaths;
}Finally, add the serialization hook: private void writeObject(ObjectOutputStream out) throws IOException {
// The resolver cannot cross the serialization boundary, so materialize
// its value at the last possible moment.
getPrevFilePaths();
out.defaultWriteObject();
}This gives you the desired lifecycle: Do not clear Update the serialization test to expect preservation: @Test
public void javaSerializationResolvesAndPreservesPrevFilePaths()
throws IOException, ClassNotFoundException {
AtomicInteger lookups = new AtomicInteger();
HoodieWriteCommitCallbackMessage message =
new HoodieWriteCommitCallbackMessage(
COMMIT_TIME, "table", "/base", updateStat(),
Option.of("commit"), Option.empty(),
() -> {
lookups.incrementAndGet();
return viewResolving(PREV_PATH);
},
Collections.emptyMap());
assertEquals(0, lookups.get());
HoodieWriteCommitCallbackMessage roundTripped =
serializeAndDeserialize(message);
assertEquals(1, lookups.get());
assertEquals(
PREV_PATH,
roundTripped.getPrevFilePaths().get("f0").getBaseFilePath());
}Keep the existing This is the unavoidable serialization boundary: the filesystem view itself cannot be shipped, so Java serialization must materialize the lazy value. JSON callbacks already do this naturally because Jackson invokes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @danny0405. I'll take this and push the two-field version. One thing to correct though:
That PR description was outdated. Nothing actually forces the behaviour. All three built-in callbacks (HTTP, Kafka, Pulsar) go through But, i agree with your choice. The class is a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, then the serialization custimization might just be the API compatibility instead of real use usage here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah that's correct. Updated now. thanks |
||
| Map<String, PrevFilePaths> paths = prevFilePaths; | ||
| if (paths == null) { | ||
| // The resolver is null only on an instance restored from Java serialization, and there | ||
| // the resolved map has already been read back into prevFilePaths (see writeObject). | ||
| paths = prevFilePathsResolver == null ? Collections.emptyMap() : prevFilePathsResolver.get(); | ||
| prevFilePaths = paths; | ||
| } | ||
| return paths; | ||
| } | ||
|
|
||
| /** | ||
| * A {@link BaseFileOnlyView} cannot cross a serialization boundary, so materialize the | ||
| * paths at the last possible moment and let the resolved map travel in their place. | ||
| */ | ||
| private void writeObject(ObjectOutputStream out) throws IOException { | ||
| getPrevFilePaths(); | ||
| out.defaultWriteObject(); | ||
| } | ||
|
|
||
| /** | ||
| * Container for previously-existing file paths associated with a single fileId in a | ||
| * commit. {@link #baseFilePath} is the base file the new write replaces, and | ||
| * {@link #bootstrapBaseFilePath} is the bootstrap-source file the previous | ||
| * base file referenced (null for non-bootstrap tables). | ||
| */ | ||
| @Getter | ||
| public static class PrevFilePaths implements Serializable { | ||
| private static final long serialVersionUID = 1L; | ||
| private final String baseFilePath; | ||
| private final String bootstrapBaseFilePath; | ||
|
|
||
| public PrevFilePaths(String baseFilePath, String bootstrapBaseFilePath) { | ||
| this.baseFilePath = baseFilePath; | ||
| this.bootstrapBaseFilePath = bootstrapBaseFilePath; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.