Commit Graph

2 Commits

Author SHA1 Message Date
Joe McDonnell
1913ab46ed IMPALA-14501: Migrate most scripts from impala-python to impala-python3
To remove the dependency on Python 2, existing scripts need to use
python3 rather than python. These commands find those
locations (for impala-python and regular python):
git grep impala-python | grep -v impala-python3 | grep -v impala-python-common | grep -v init-impala-python
git grep bin/python | grep -v python3

This removes or switches most of these locations by various means:
1. If a python file has a #!/bin/env impala-python (or python) but
   doesn't have a main function, it removes the hash-bang and makes
   sure that the file is not executable.
2. Most scripts can simply switch from impala-python to impala-python3
   (or python to python3) with minimal changes.
3. The cm-api pypi package (which doesn't support Python 3) has been
   replaced by the cm-client pypi package and interfaces have changed.
   Rather than migrating the code (which hasn't been used in years), this
   deletes the old code and stops installing cm-api into the virtualenv.
   The code can be restored and revamped if there is any interest in
   interacting with CM clusters.
4. This switches tests/comparison over to impala-python3, but this code has
   bit-rotted. Some pieces can be run manually, but it can't be fully
   verified with Python 3. It shouldn't hold back the migration on its own.
5. This also replaces locations of impala-python in comments / documentation /
   READMEs.
6. kazoo (used for interacting with HBase) needed to be upgraded to a
   version that supports Python 3. The newest version of kazoo requires
   upgrades of other component versions, so this uses kazoo 2.8.0 to avoid
   needing other upgrades.

The two remaining uses of impala-python are:
 - bin/cmake_aux/create_virtualenv.sh
 - bin/impala-env-versioned-python
These will be removed separately when we drop Python 2 support
completely. In particular, these are useful for testing impala-shell
with Python 2 until we stop supporting Python 2 for impala-shell.

The docker-based tests still use /usr/bin/python, but this can
be switched over independently (and doesn't impact impala-python)

Testing:
 - Ran core job
 - Ran build + dataload on Centos 7, Redhat 8
 - Manual testing of individual scripts (except some bitrotted areas like the
   random query generator)

Change-Id: If209b761290bc7e7c716c312ea757da3e3bca6dc
Reviewed-on: http://gerrit.cloudera.org:8080/23468
Reviewed-by: Michael Smith <michael.smith@cloudera.com>
Tested-by: Michael Smith <michael.smith@cloudera.com>
2025-10-22 16:30:17 +00:00
stiga-huang
777ae104bb IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.

thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.

Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
 - A required field is deleted in an existing struct.
 - A new required field is added in an existing struct.
 - An existing field is renamed.
 - The qualifier (required/optional) of a field is changed.
 - The type of a field is changed.
 - An enum item is removed.
 - Enum items are reordered.

Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.

Warning examples for commit e48af8c04:
  "common/thrift/StatestoreService.thrift": [
   {
    "message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 345,
    "side": "REVISION"
   }
  ]

Warning examples for commit 595212b4e:
  "common/thrift/CatalogObjects.thrift": [
   {
    "message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 612,
    "side": "REVISION"
   }
  ]

Warning examples for commit c57921225:
  "common/thrift/CatalogObjects.thrift": [
   {
    "message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 606,
    "side": "REVISION"
   }
  ],
  "common/thrift/CatalogService.thrift": [
   {
    "message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 215,
    "side": "REVISION"
   },
   {
    "message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 209,
    "side": "REVISION"
   }
  ],
  "common/thrift/Query.thrift": [
   {
    "message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 876,
    "side": "REVISION"
   }
  ]

Warning example for commit 2b2cf8d96:
  "common/thrift/CatalogService.thrift": [
   {
    "message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 381,
    "side": "REVISION"
   }
  ]

Warning example for commit c01efd096:
  "common/thrift/JniCatalog.thrift": [
   {
    "message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 107,
    "side": "PARENT"
   }
  ]

Warning example for commit 374783c55:
"common/thrift/Query.thrift": [
   {
    "message": "Changing type of field 'enabled_runtime_filter_types' from PlanNodes.TEnabledRuntimeFilterTypes to set<PlanNodes.TRuntimeFilterType> in TQueryOptions might break the compatibility between impalad and catalogd/statestore during upgrade",
    "line": 449,
    "side": "REVISION"
   }

Tests
 - Add tests in tests/infra/test_thrift_parser.py
 - Verified the script with all(1260) commits of common/thrift.

Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Reviewed-on: http://gerrit.cloudera.org:8080/22264
Reviewed-by: Riza Suminto <riza.suminto@cloudera.com>
Reviewed-by: Michael Smith <michael.smith@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
2025-01-07 02:00:17 +00:00