Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/10543 )
Change subject: IMPALA-6119: Fix issue with multiple partitions sharing same location ...................................................................... Patch Set 18: (9 comments) http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py File tests/metadata/test_partition_metadata.py: http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@124 PS18, Line 124: # Create three partitions where two of them point to the same location. : self.filesystem_client.make_dir(TBL_NAME + '/p1') : self.filesystem_client.make_dir(TBL_NAME + '/p2') This looks wrong, because the directories made here will only be "same_loc_test/{p1,p2}" and not rooted at TBL_LOCATION. Are these lines needed? And if they are needed, then they need to have a uniqueness to them, because as it stands, parallel tests will overwrite each other's directories in this spot. Include unique_database as part of the directory name to solve this problem. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@127 PS18, Line 127: execute Here and elsewhere, there's a execute_query_expect_success() method you should consider instead for statements you expect always to succeed. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@163 PS18, Line 163: it's Are we (Impala) in control of this message? It should be "its". http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@168 PS18, Line 168: assert false If we are here, then we never raised an exception. It would be polite to show the partition information as part of diagnostics when the test fails. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@177 PS18, Line 177: pytest.skip() You can insert a reason here. It's mildly helpful to someone that happens to see this go by on a console. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@184 PS18, Line 184: # Cleanup any existing data in the table directory. : self.filesystem_client.delete_file_dir(TBL_NAME, recursive=True) As before, I can't tell that this is needed. As before, if needed, this can collide with tests that are using a directory of TBL_NAME in L125-126, assuming the pwd is the same. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@187 PS18, Line 187: execute As before, execute_query_expect_success() in places where you expect success. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@191 PS18, Line 191: # Create two partitions pointing to the same location. : self.filesystem_client.make_dir(TBL_NAME + '/p1') Same comment for L185 applies here. http://gerrit.cloudera.org:8080/#/c/10543/18/tests/metadata/test_partition_metadata.py@202 PS18, Line 202: hive_cmd = ("ALTER TABLE %s DROP PARTITION (j=1)" % FQ_TBL_NAME) : call(["hive", "-e", hive_cmd]) You can use self.hive_client() instead. -- To view, visit http://gerrit.cloudera.org:8080/10543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a54bc8224bcefe65b83de2df58bb84629f2aa4a Gerrit-Change-Number: 10543 Gerrit-PatchSet: 18 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 22 Jun 2018 16:00:59 +0000 Gerrit-HasComments: Yes
