Yingyi Bu has posted comments on this change. Change subject: Ensure Metadata locks are acquired for SqlPP queries ......................................................................
Patch Set 6: (5 comments) It seems the core change is to do metadata locking in the Metadata.findDataset() method. Since MetadataLockManager uses ReentrantLocks, it looks that we don't need to do lock existence checks and to use tryLock in MetadataLockManager can save a lot of lock existence check code. Can we let the change be minimum? It seems that only changing MetadataProvider.findDataset() and make all dataset enquiries go through MetadataProvider is sufficient to fix the issue? It seems that we don't necessarily need to change the current parser/AST implementations. https://asterix-gerrit.ics.uci.edu/#/c/1642/5/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java: PS5, Line 121: MetadataManager.INSTANCE.getDataset(...) is still called at several other places. https://asterix-gerrit.ics.uci.edu/#/c/1642/6/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/Query.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/Query.java: PS6, Line 39: datasets Is this parameter still necessary? https://asterix-gerrit.ics.uci.edu/#/c/1642/5/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java: PS5, Line 2095: lockedDatasets Can we let lockedDataset be a field of MetadataProvider? Setting the set from the outside makes the ownership of the set reference difficult to track. Actually, for simplicity, it looks we can remove lockedDataset for now -- it's not a problem if we lock multiple times as MetadataLockManager uses ReentrantLocks. https://asterix-gerrit.ics.uci.edu/#/c/1642/5/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SplitsAndConstraintsUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SplitsAndConstraintsUtil.java: PS5, Line 59: Dataset minimize the signature? dataverseName and datasetName are part of dataset, so as temp. PS5, Line 96: minimize the signature? dataverseName and datasetName are part of dataset, so as temp. -- To view, visit https://asterix-gerrit.ics.uci.edu/1642 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f468599897a37cbcb12d8577d072f340f0d949c Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
