Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/23063 )
Change subject: [thrift] Explicitly specify field ID to silence protocol compatibility warning ...................................................................... Patch Set 1: Code-Review-1 Thank you for looking into this! On the top of the hive_metastore.thrift file we have the following line: # DO NOT MODIFY! Copied from # https://raw.githubusercontent.com/apache/hive/rel/release-3.1.1/standalone-metastore/src/main/thrift/hive_metastore.thrift So I took some time to investigate. If we take a look at the generated sources in the current state, the auto-assigned field ID will be -1 for o2! grep -A 50 "create_ischema_result.*read" src/kudu/hms/ThriftHiveMetastore.cpp -> switch (fid) { case 1: if (ftype == ::apache::thrift::protocol::T_STRUCT) { xfer += this->o1.read(iprot); this->__isset.o1 = true; } else { xfer += iprot->skip(ftype); } break; case -1: if (ftype == ::apache::thrift::protocol::T_STRUCT) { xfer += this->o2.read(iprot); this->__isset.o2 = true; } else { xfer += iprot->skip(ftype); } break; case 3: if (ftype == ::apache::thrift::protocol::T_STRUCT) { xfer += this->o3.read(iprot); this->__isset.o3 = true; } else { xfer += iprot->skip(ftype); } break; default: xfer += iprot->skip(ftype); break; } With the current patch the ID will be 2. This means old HMS clients expecting field ID -1 and new HMS clients sending field ID 2 would not be able to communicate properly, potentially breaking rolling upgrades or mixed-version environments. If we want to eliminate the warning while preserving compatibility, we could explicitly specify the current auto-assigned field ID to -1. This would: - Silence the compiler warning (field ID is now explicit) - Maintain wire protocol compatibility (still uses field ID -1) - Make the current behavior intentional rather than accidental However, since this file says 'DO NOT MODIFY' and comes from upstream Hive, we should consider whether any change is appropriate, or if we should find a way to suppress the warning instead. -- To view, visit http://gerrit.cloudera.org:8080/23063 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifdb8b650f9b8ed1cdea9dd0cf1a80ef264ccc7d7 Gerrit-Change-Number: 23063 Gerrit-PatchSet: 1 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 23 Jun 2025 11:51:08 +0000 Gerrit-HasComments: No
