Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17955 )
Change subject: IMPALA-10967 Load data should handle AWS NLB-type timeout ...................................................................... Patch Set 6: (3 comments) Thanks for adding the tests and query option. This is looking good, I only have a couple small comments. http://gerrit.cloudera.org:8080/#/c/17955/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17955/6/be/src/service/client-request-state.cc@769 PS6, Line 769: DebugActionNoFail( : exec_request_->query_options, "CRS_DELAY_BEFORE_LOAD_DATA"); Nit: My only thought here is that I do like it when these statements are right next to the statement that we are simulating the delay about (in this case frontend_->LoadData()). http://gerrit.cloudera.org:8080/#/c/17955/6/tests/metadata/test_load.py File tests/metadata/test_load.py: http://gerrit.cloudera.org:8080/#/c/17955/6/tests/metadata/test_load.py@111 PS6, Line 111: class TestAsyncLoadData(TestLoadData): One thing about subclassing TestLoadData is that TestAsyncLoadData will get its own copy of test_load() from TestLoadData. When those copies execute in parallel, things can go wrong. One way out is to create a TestLoadDataBase that contains the pieces you need to share, and then subclass for both TestLoadData and TestAsyncLoadData. http://gerrit.cloudera.org:8080/#/c/17955/6/tests/metadata/test_load.py@122 PS6, Line 122: @pytest.mark.execute_serially # To avoid file copy failure: dst file does not exist Nice to have: When possible, we want to structure tests to allow them to execute in parallel. I ran test_async_load locally in its 6 variations, and it took about 6 minutes. I think a decent chunk of that was setup/teardown and not the test itself. In this case, it would involve replacing STAGING_PATH with something under the unique_database directory (and populating it with some files, etc). Unfortunately, unique_database doesn't really work with setup_method/teardown_method, so it would need some rework of populating the directory. -- To view, visit http://gerrit.cloudera.org:8080/17955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c2437e9894510204303ec07710cad60102c8821 Gerrit-Change-Number: 17955 Gerrit-PatchSet: 6 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Mon, 25 Oct 2021 21:51:31 +0000 Gerrit-HasComments: Yes
