Quanlong Huang has uploaded this change for review. (
http://gerrit.cloudera.org:8080/22264
Change subject: IMPALA-13305: Better thrift compatability checks based on
pyparsing
......................................................................
IMPALA-13305: Better thrift compatability 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 gramma 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.
The thrift_parser is used 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": "REVISION"
}
]
Tests
- 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
2 files changed, 305 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/1
--
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: newchange
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <[email protected]>
