Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13317 )
Change subject: Allow alter legacy tables in Hive Metastore Kudu plugin ...................................................................... Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13317/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/13317/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@137 PS2, Line 137: // Allow non-Kudu tables to be altered, but ensure that the alteration : // isn't introducing Kudu-specific properties. This includes legacy tables, : // however excludes upgrading case (altering legacy Kudu table to be Kudu table). : if (!isKuduTable(oldTable) && : !(isLegacyKuduTable(oldTable) && isKuduTable(newTable))) { : checkNoKuduProperties(newTable); : return; : } I'm finding this difficult to reason about without enumerating every case. Would something like this be sufficient: if (!isKuduTable(oldTable) && !isLegacyKuduTable(oldTable) && !isKuduTable(newTable) && !isLegacyKuduTable(newTable)) { // This isn't a Kudu table. checkNoKuduProperties(newTable); return; } if (isLegacyKuduTable(oldTable) && isLegacyKuduTable(newTable)) { // We're altering a legacy table and therefore don't care (but with an actual explanation). return; } if (isKuduTable(oldTable) && isLegacyKuduTable(newTable)) { checkKuduProperties(newTable); return; } if (isKuduTable(newTable)) { checkKuduProperties(newTable); } ... I'd prefer this style because it makes reasoning about oldTable and newTable very straightforward IMO. As you read through the code, invariants about the state of the alter operation are known, rather than the states of each table (which I think are harder to reason about). This isn't a full implementation, but does that style make sense? If so, could you update this function to use it? -- To view, visit http://gerrit.cloudera.org:8080/13317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie76ce2cd7dda0b4391e91abe2da2801d305a64d3 Gerrit-Change-Number: 13317 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 14 May 2019 19:17:14 +0000 Gerrit-HasComments: Yes
