Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14444 )
Change subject: IMPALA-8648: Add stress tests for ACID INSERTs/SELECTs ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/14444/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14444/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1854 PS2, Line 1854: tryLock(table, "truncating"); As we have discussed in person, this locking only protects against the specific case when a table is upgraded in Impala from non-ACID to ACID after the call to getExistingTable(). I think we generally do not handle this case. I am ok with either keeping or removing this locking, but if we keep it, then some comments about its purpose would be nice. http://gerrit.cloudera.org:8080/#/c/14444/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1873 PS2, Line 1873: Preconditions.checkState(!catalog_.getLock().isWriteLockedByCurrentThread()); This looks a bit dangerous - if we throw an exception when we have the catalog lock, then it won't be released. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py File tests/stress/test_acid_stress.py: http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@164 PS2, Line 164: Task(self._hive_role_write_inserts, tbl_name, partitioned), nit: here and at other places: doesn't this need indentation of 4? http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@219 PS2, Line 219: tbl_name, wid, num_inserts)) nit: +2 indent http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@223 PS2, Line 223: except Exception as e: : # Some transactions might fail due to high contention. : print str(e) : if "TransactionException" not in str(e): : raise e I think it would be better not to swallow these exceptions if possible. If truncate would get the HMS lock before the catalog's table lock, then the "big waits" (when TRUNCATE waits for exclusive lock or INSERT's shared lock is blocked by a TRUNCATE's exclusive lock) would happen when waiting for HMS locks, which seems more normal than waiting long for catalog locks, and should have longer timeouts. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@314 PS2, Line 314: "TransactionException" not in str(e) and : "CLIENT_REQUEST_UPDATE_CATALOG" not in str(e) Same as line 223. CLIENT_REQUEST_UPDATE_CATALOG errors could be accepted only if the debug action was inserted. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@343 PS2, Line 343: self.client.execute("""create table %s (i int) TBLPROPERTIES ( Similarly to TestAcidInsertsBasic, I think it would be nice to run the same tests on partitioned tables, e.g. with writing to different partitions randomly. This seems like a "cheap" way to cover more code. I am ok with leaving this as a TODO. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@353 PS2, Line 353: checker nit: this could be "checkers", as there can be several of them http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@354 PS2, Line 354: num_writers) nit: would fit to last line -- To view, visit http://gerrit.cloudera.org:8080/14444 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I066652bfa7d924742af01aef8df4512e00620c7d Gerrit-Change-Number: 14444 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 21 Oct 2019 15:16:02 +0000 Gerrit-HasComments: Yes
