yihua commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1122164561
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -151,30 +156,38 @@ public void stop() {
* Determines if local view of table's timeline is behind that of client's
view.
*/
private boolean isLocalViewBehind(Context ctx) {
- String basePath =
ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
- String lastKnownInstantFromClient =
- ctx.queryParamAsClass(RemoteHoodieTableFileSystemView.LAST_INSTANT_TS,
String.class).getOrDefault(HoodieTimeline.INVALID_INSTANT_TS);
- String timelineHashFromClient =
ctx.queryParamAsClass(RemoteHoodieTableFileSystemView.TIMELINE_HASH,
String.class).getOrDefault("");
- HoodieTimeline localTimeline =
-
viewManager.getFileSystemView(basePath).getTimeline().filterCompletedOrMajorOrMinorCompactionInstants();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Client [ LastTs=" + lastKnownInstantFromClient + ",
TimelineHash=" + timelineHashFromClient
- + "], localTimeline=" + localTimeline.getInstants());
- }
+ try {
+ // This read lock makes sure that if the local view of the table is
being synced,
+ // no timeline server requests should be processed or handled until the
sync process
Review Comment:
The `synchronized` won't be executed if the timeline is already refreshed
and this method returns `false` to skip the synchronized block, but in such a
case the metadata file system view might not have been fully updated, causing
the issue.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/AbstractTableFileSystemView.java:
##########
@@ -282,6 +286,20 @@ public void reset() {
}
}
+ /**
+ * Resets the view states, which can be overridden by subclasses. This
reset logic is guarded
+ * by the write lock.
+ * <p>
+ * NOTE: This method SHOULD BE OVERRIDDEN for any custom logic. DO NOT
OVERRIDE
+ * {@link AbstractTableFileSystemView#reset} directly, which may cause stale
file system view
+ * to be served.
+ */
+ protected void runReset() {
Review Comment:
Yes, `reset()` should call `runReset()`. Not sure why the change is
reverted somehow.
I don't think we should change the naming of `sync` and `reset` in the
interface `SyncableFileSystemView` as they can also have a different
implementation. I've added documentation to make sure how the custom logic
should be overridden.
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -187,12 +200,20 @@ private boolean syncIfLocalViewBehind(Context ctx) {
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
synchronized (view) {
if (isLocalViewBehind(ctx)) {
- HoodieTimeline localTimeline =
viewManager.getFileSystemView(basePath).getTimeline();
- LOG.info("Syncing view as client passed last known instant " +
lastKnownInstantFromClient
- + " as last known instant but server has the following last
instant on timeline :"
- + localTimeline.lastInstant());
- view.sync();
- return true;
+ try {
Review Comment:
As @nsivabalan mentioned, this is not thread-safe before, as some requests
may completely skip the synchronized block as `isLocalViewBehind` returns false
while the `view.sync()` is still in progress, and the read-write lock does not
prevent the stale file system view to be served.
##########
hudi-common/src/test/java/org/apache/hudi/metadata/HoodieBackedTestDelayedTableMetadata.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.hudi.metadata;
+
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.engine.HoodieEngineContext;
+
+import org.apache.log4j.LogManager;
+import org.apache.log4j.Logger;
+
+/**
+ * Table metadata provided by an internal DFS backed Hudi metadata table,
+ * with an intentional delay in `reset()` to test concurrent reads and writes.
+ */
+public class HoodieBackedTestDelayedTableMetadata extends
HoodieBackedTableMetadata {
Review Comment:
Generally a good idea to have a separate class for a notable test logic.
--
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]
