Zoltan Borok-Nagy 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 3: (11 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: } > As we have discussed in person, this locking only protects against the spec Added comments about it. http://gerrit.cloudera.org:8080/#/c/14444/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1873 PS2, Line 1873: newCatalogVersion = truncateTransactionalTable(params, table); > This looks a bit dangerous - if we throw an exception when we have the cata You are right, fixed it here and other places in this file. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14444/2/tests/common/impala_test_suite.py@219 PS2, Line 219: # > flake8: E265 block comment should start with '# ' Done 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@48 PS2, Line 48: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@164 PS2, Line 164: run_tasks([ > nit: here and at other places: doesn't this need indentation of 4? Done http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@219 PS2, Line 219: impalad_client.execute("insert into table %s values (%i, %i)" % ( > nit: +2 indent Done http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@223 PS2, Line 223: impalad_client.execute("truncate table %s" % tbl_name) : except Exception as e: : # Some transactions might fail due to high contention. : # print str(e) : # if "Tra > I think it would be better not to swallow these exceptions if possible. In this PS when truncate tries to acquire the HMS lock it already released the catalog's table lock. And after it acquired the HMS lock it tries to acquire the catalog table lock again. I did a closer look at why we get this exception, and it turned out that setting DataOperationType to DELETE for truncate was not the best choice because HMS's conflict detection kicked in and aborted some transactions. Now I set DataOperationType to NO_TXN (which is still not the best because we do have a transaction) and things work just fine. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@314 PS2, Line 314: if should_succeed or "CLIENT_REQUEST_UPDATE_CATALOG" not in str(e): : raise e > Same as line 223. CLIENT_REQUEST_UPDATE_CATALOG errors could be accepted on Removed TransactionException, and added 'or val == 1'. http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@343 PS2, Line 343: num_writers = 3 > Similarly to TestAcidInsertsBasic, I think it would be nice to run the same Done http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@353 PS2, Line 353: ytest.m > nit: this could be "checkers", as there can be several of them Done http://gerrit.cloudera.org:8080/#/c/14444/2/tests/stress/test_acid_stress.py@354 PS2, Line 354: @pytest.mark.stress > nit: would fit to last line Done -- 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: 3 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 24 Oct 2019 16:37:47 +0000 Gerrit-HasComments: Yes
