Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10291 )
Change subject: IMPALA-6948,IMPALA-6962: add end-to-end tests ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/10291/3/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/10291/3/be/src/catalog/catalog-server.cc@304 PS3, Line 304: long current_catalog_version; > Why is this dependent on the success of GetDbs()? I think the version and t did it this way to share the error handling and avoid multiple "error" fields. I could have an error field and a list of reasons? http://gerrit.cloudera.org:8080/#/c/10291/3/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/10291/3/tests/common/impala_service.py@151 PS3, Line 151: """ Extracts and returns the version of the catalog object's 'thrift_txt' representation.""" > I think the JSON field is called 'thrift_string' the value that passed in here is coming from get_catalog_object_dump, which is just html, not json. http://gerrit.cloudera.org:8080/#/c/10291/3/tests/custom_cluster/test_metadata_replicas.py File tests/custom_cluster/test_metadata_replicas.py: http://gerrit.cloudera.org:8080/#/c/10291/3/tests/custom_cluster/test_metadata_replicas.py@39 PS3, Line 39: @pytest.mark.xfail(reason="IMPALA-6948") > Issue is fixed now. Done http://gerrit.cloudera.org:8080/#/c/10291/3/tests/custom_cluster/test_metadata_replicas.py@54 PS3, Line 54: "create table if not exists %s.x (a string)" % db_name) > We should not do IF NOT EXISTS here and IF EXISTS below in the drop. If the Done http://gerrit.cloudera.org:8080/#/c/10291/3/tests/custom_cluster/test_metadata_replicas.py@55 PS3, Line 55: cursor.execute("set SYNC_DDL=true") > Not obvious why we need this. Can we remove it if we consistently use the s Done http://gerrit.cloudera.org:8080/#/c/10291/3/tests/custom_cluster/test_metadata_replicas.py@57 PS3, Line 57: assert "x" in self.client.execute("show tables in %s" % db_name).data > The use of both cursor and self.client is somewhat confusing. How many conn Done -- To view, visit http://gerrit.cloudera.org:8080/10291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6c5b39e29b2885cd30fede18833cbf23fb755f5 Gerrit-Change-Number: 10291 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 09 May 2018 00:26:00 +0000 Gerrit-HasComments: Yes