924060929 commented on code in PR #68042:
URL: https://github.com/apache/doris/pull/68042#discussion_r4214297024
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/doris/RemoteDorisExternalTable.java:
##########
@@ -62,62 +62,58 @@ protected synchronized void makeSureInitialized() {
}
private RemoteOlapTable getDorisOlapTable() {
- if (!isSyncOlapTable) {
- synchronized (this) {
- if (!isSyncOlapTable) {
- try {
- isSyncOlapTable = true;
- remoteOlapTable = null;
- lastException = null; // clear previous exception
-
- List<Partition> cachedPartitions =
Lists.newArrayList(partitions);
- List<Partition> cachedTempPartitions =
Lists.newArrayList(tempPartitions);
- RemoteOlapTable olapTable =
((RemoteDorisExternalCatalog) catalog).getFeServiceClient()
- .getOlapTable(dbName, remoteName, tableId,
cachedPartitions, cachedTempPartitions);
- olapTable.setCatalog((RemoteDorisExternalCatalog)
catalog);
- olapTable.setDatabase((RemoteDorisExternalDatabase)
db);
-
- // Remove redundant nested synchronized block
- tableId = olapTable.getId();
- partitions =
Lists.newArrayList(olapTable.getPartitions());
- tempPartitions =
Lists.newArrayList(olapTable.getTempPartitions().getPartitions());
Review Comment:
The shorter monitor scope is appropriate for the contention problem. The
remaining concern is specifically read-after-write, not whether a query already
running concurrently with INSERT may read an older snapshot.
The sequential INSERT/SELECT explanation misses an unrelated refresh on the
same table instance:
1. Session A finishes INSERT planning and executes the write.
2. Session B starts a metadata refresh and the remote FE snapshots version
V. The remote table read lock is released, but response transport/local
reconstruction has not finished.
3. A's INSERT returns successfully with the transaction VISIBLE at V+1.
4. A starts a new SELECT. Because B's task is still unfinished, SELECT joins
B and receives V.
This can be constructed with a single remote master FE and SQL cache
disabled; it does not require follower lag. In the previous implementation, B
held the same monitor required by makeSureInitialized() throughout refresh. A's
later SELECT therefore had to wait before selecting a refresh and would issue a
post-commit RPC. The lock's original intent does not change that behavioral
difference.
A possible fix is to keep refresh work outside the monitor but close a
task's admission before its metadata RPC starts. Requests arriving afterward
can share a pending successor task, which runs after the current refresh
finishes. This batches requests while preventing late callers from consuming an
earlier snapshot, and keeps partition-cache updates serialized. Invalidating
only after local INSERT would not cover writes through other connections to the
remote cluster.
The new sharing/retry tests are useful, so the earlier statement that there
is no upstream unit coverage is now outdated. Please add a latch-controlled
V-to-V+1 test for the schedule above, and ensure an interrupted waiter cannot
leave the successor task without a runner.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
