Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#10).
Change subject: IMPALA-13305: Better thrift compatibility checks based on
pyparsing
......................................................................
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
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
M infra/python/deps/requirements.txt
A tests/infra/test_thrift_parser.py
4 files changed, 403 insertions(+), 95 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/10
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Riza Suminto <[email protected]>
