vinothchandar commented on code in PR #8079:
URL: https://github.com/apache/hudi/pull/8079#discussion_r1122642714
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -78,6 +79,10 @@ public class RequestHandler {
private final BaseFileHandler dataFileHandler;
private final MarkerHandler markerHandler;
private final Registry metricsRegistry =
Registry.getRegistry("TimelineService");
+ // This read-write lock is used for syncing the file system view if it is
behind client's view
Review Comment:
We have locking at two levels now. Here. and then inside
AbstractTableFileSystemView. Could we simplify this.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/AbstractTableFileSystemView.java:
##########
@@ -269,19 +269,35 @@ public void close() {
/**
* Clears the partition Map and reset view states.
+ * <p>
+ * NOTE: This method SHOULD NOT BE OVERRIDDEN which may cause stale file
system view
+ * to be served. Instead, override {@link
AbstractTableFileSystemView#runReset} to
+ * add custom logic.
*/
@Override
public void reset() {
Review Comment:
make this `final`? so it cant be overridden? why add comment :)
##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/AbstractTableFileSystemView.java:
##########
@@ -269,19 +269,35 @@ public void close() {
/**
* Clears the partition Map and reset view states.
+ * <p>
+ * NOTE: This method SHOULD NOT BE OVERRIDDEN which may cause stale file
system view
+ * to be served. Instead, override {@link
AbstractTableFileSystemView#runReset} to
+ * add custom logic.
*/
@Override
public void reset() {
try {
writeLock.lock();
- clear();
- // Initialize with new Hoodie timeline.
- init(metaClient, getTimeline());
+ runReset();
} finally {
writeLock.unlock();
}
}
+ /**
+ * 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
Review Comment:
then make it abstract? the interface design here should force the
implementations to do the right thing.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataFileSystemView.java:
##########
@@ -90,14 +90,14 @@ protected Map<Pair<String, Path>, FileStatus[]>
listPartitions(List<Pair<String,
}
@Override
- public void reset() {
- super.reset();
Review Comment:
instead of all this. Can we make this impl just use the writelock here as
well? and move on? i.e make the `writeLock` protected. and call the code in
`super.reset()` here inline.
##########
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:
yeah this seems like a good use of read/write locks.. but then, the whole
thing can be inside a synchronized block for the qps levels we deal with here,
I feel. Still - don't think this is an anti pattern per se.
##########
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:
I wonder how much perf is actually saved by checking for
`isLocalViewBehind(ctx)` outside the synchronized block? We can just do a
simple, single synchronized block
```
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;
}
}
```
read-write locks are going to add cognitive overhead everytime we read this
:)
--
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]
