[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1125407040
##
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##
@@ -181,22 +181,24 @@ private boolean isLocalViewBehind(Context ctx) {
* Syncs data-set view if local view is behind.
*/
private boolean syncIfLocalViewBehind(Context ctx) {
-if (isLocalViewBehind(ctx)) {
- String basePath =
ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
- String lastKnownInstantFromClient =
ctx.queryParamAsClass(RemoteHoodieTableFileSystemView.LAST_INSTANT_TS,
String.class).getOrDefault(HoodieTimeline.INVALID_INSTANT_TS);
- 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;
-}
+boolean result = false;
+String basePath =
ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
+SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
+synchronized (view) {
+ if (isLocalViewBehind(ctx)) {
+
+String lastKnownInstantFromClient = ctx.queryParamAsClass(
+RemoteHoodieTableFileSystemView.LAST_INSTANT_TS, String.class)
+.getOrDefault(HoodieTimeline.INVALID_INSTANT_TS);
+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();
+result = true;
Review Comment:
Can we return directly from this line?
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1122370993
##
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.
+ *
+ * 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:
Don’t think the doc makes any sense, let’s give them better name.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1122369457
##
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:
Did you mean when the local view is not behind, it still needs to wait for
the next refresh? Sorry I didn’t think that is the correct direction to go.
And there is another question you didn’t answer, why it is okey when MDT is
disabled? They share the same lock guard right?
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1121420324
##
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:
cc @nbalajee Do you have interest for the reviewing?
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1121419697
##
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:
All the code block is thread safe already because of the `synchronized
(view)`.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1121418704
##
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:
Don't think we need a explicit lock obj again, the whole code block is
guarded by a `synchronized` object lock.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hudi] danny0405 commented on a diff in pull request #8079: [HUDI-5863] Fix HoodieMetadataFileSystemView serving stale view at the timeline server
danny0405 commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1121416847
##
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.
+ *
+ * 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:
Guess the method should be guarded by reset `writeLock` ? `#reset` should
invoke `#runReset` instead I guess.
And can we give these methods more straight-forward names?
Like sync -> syncThreadSafely, runSync -> sync, reset->resetThreadSafely,
runReset -> reset
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
