danny0405 commented on code in PR #18988:
URL: https://github.com/apache/hudi/pull/18988#discussion_r3687475410
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/callback/common/HoodieWriteCommitCallbackMessage.java:
##########
@@ -69,10 +73,91 @@ public class HoodieWriteCommitCallbackMessage implements
Serializable {
*/
private final Option<Map<String, String>> extraMetadata;
+ /**
+ * Previous base file paths keyed by fileId, derived lazily 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>Resolution is deferred until the first {@link #getPrevFilePaths()}
call: a callback
+ * that never reads the previous paths pays nothing (no FileSystemView
access). Transient
+ * because it captures a FileSystemView supplier, which is not serializable
- these paths
+ * are JVM-local derived state, so a message restored from Java
serialization reports none.
+ * Excluded from the generated getters so the {@link Lazy} wrapper never
leaks into JSON.
+ */
+ @Getter(AccessLevel.NONE)
+ private final transient Lazy<Map<String, PrevFilePaths>> prevFilePaths;
+
+ /**
+ * 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,
+ 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.prevFilePaths = 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. A consumer that never calls this triggers no
FileSystemView lookup.
+ * Never null: empty when no view was supplied, when the commit only
inserted, or when this
+ * message was restored from Java serialization.
+ */
+ public Map<String, PrevFilePaths> getPrevFilePaths() {
Review Comment:
sorry sagar, I asked Codex and it seems the Ser/De customization is
unavoidable:
```markdown
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:
```java
this.prevFilePaths = null;
this.prevFilePathsResolver = Lazy.lazily(() ->
HoodieWriteCommitCallbackUtil.resolvePrevFilePaths(
hoodieWriteStat,
fsViewSupplier == null ? null : fsViewSupplier.get()));
```
The getter memoizes the resolved value:
```java
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:
```java
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:
```text
message construction
↓ no filesystem-view access
callback ignores prevFilePaths
↓ still no access
getPrevFilePaths() or JSON/Java serialization
↓ resolve once and memoize
deserialized message
↓ resolved map remains available; resolver is null
```
Do not clear `prevFilePathsResolver` manually after resolution. Hudi’s
`Lazy` already clears its initializer, and keeping the `Lazy` reference avoids
subtle concurrency races.
Update the serialization test to expect preservation:
```java
@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 `serialVersionUID`. For compatibility with streams
produced before this field existed, the getter’s `resolver == null` fallback
returns an empty map.
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
`getPrevFilePaths()` while generating the payload. See the current [[PR
changes](https://github.com/apache/hudi/pull/18988/changes)](https://github.com/apache/hudi/pull/18988/changes).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
