[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Reviewed-on: http://gerrit.cloudera.org:8080/16388 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 07 Oct 2020 17:44:21 + Gerrit-HasComments: No
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#10). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/10 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > I'm not sure how the HMS manages its credentials, but it'd be good to at le I hit some issues writing an automated test that validates renewal is handled by the HMS, but I did manually validate this in a real environment with a short ticket and renew lifetime. I would like to commit this patch and work on adding a test later to unblock work. http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@a52 PS9, Line 52: > Missed this before. Why don't we want to retry these tests anymore? Should Ah, right. I was expecting to change this and forgot to add it back. Good catch. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Oct 2020 23:36:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > Good to know that HMS service takes care of that. I'm not sure how the HMS manages its credentials, but it'd be good to at least verify with a HMS expert that the plugin runs in an environment that does periodically re-login. I looked around the Hive codebase a bit, but not enough to find any background re-logins. I did stumble across another MetaStoreEventListener https://github.com/apache/hive/blob/48c01107cd18d80867369e5addfa1fc5b4e7f698/hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java and I don't see logins, but I'm also not sure if the Hadoop fs APIs they are using log in automatically. http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@a52 PS9, Line 52: Missed this before. Why don't we want to retry these tests anymore? Should we be using a KuduTestHarness here? -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 27 Sep 2020 21:00:33 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > I didn't test this explicitly, but because I am using the credentials from Good to know that HMS service takes care of that. Do we know that it works as expected for Kudu client? BTW, if it possible to add such a test as a part of MiniHms in ExternalMiniCluster? There it will be easy to play with Kerberos-related settings using MiniKdc properties. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 19:33:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@23 PS8, Line 23: Additonally > Additionally Done http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > For the Kerberos-enabled case I'm curious how the expiration of authn crede I didn't test this explicitly, but because I am using the credentials from the HMS service itself, I would expect that the HMS would be renewing and updating the subject. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 19:09:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster For the Kerberos-enabled case I'm curious how the expiration of authn credentials is handled there. IIRC, Kudu client itself doesn't have provisions to renew Kerberos tickets nor it accepts a keytab. It's assumed that a Kudu client re-uses creds from the Kerberos cache (which is supposed to be initialized externally by other actors). So, the concern here is that when Kerberos creds in the cache expire, this unexpectedly stops working. And if so, would it create some problems in the consistency of catalog data on its own. For example, something that would require a manual intervention to restore the consistency. Did you try to run this if using very short-lived kerberos tickets? I guess that's something about setting 'ticket_lifetime' and 'renew_lifetime' to few minutes in krb5.conf -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:58:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#9). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additionally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/9 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@33 PS8, Line 33: - > nit: drop the extra dash ? I meant this as a sub-item. http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@34 PS8, Line 34: CREATE/ALTER/DROP on sync cluster > Does it work in case of using Ranger along for fine-grained authz? It does, though I didn't explicitly test because the superuser configs bypass ranger authz. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:23:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@23 PS8, Line 23: Additonally Additionally http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@33 PS8, Line 33: - nit: drop the extra dash ? http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@34 PS8, Line 34: CREATE/ALTER/DROP on sync cluster Does it work in case of using Ranger along for fine-grained authz? -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:18:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has removed a vote on this change. Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Sep 2020 21:36:39 + Gerrit-HasComments: No
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#8). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/8 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has removed a vote on this change. Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Removed Verified+1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/16388 )
Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization
is enabled
..
Patch Set 7: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16388/1/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/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@164
PS1, Line 164: return
> I don't think we need to, this is drop table so it's not introducing proper
Ack
http://gerrit.cloudera.org:8080/#/c/16388/1/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@172
PS1, Line 172: if (!isKuduTable(table)) {
: // However, make sure it doesn't have a table id from the
context.
: // The table id is only meant for Kudu tables and set by
the Kudu master.
:
> This is the case when the drop table request doesn't have target table id i
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37
Gerrit-Change-Number: 16388
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 14 Sep 2020 21:56:25 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 11 Sep 2020 20:09:16 + Gerrit-HasComments: No
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16388 )
Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization
is enabled
..
Patch Set 7: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16388/7/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/16388/7/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@478
PS7, Line 478: // If the request is from the Kudu Master, we know HMS sync
is enabled
: // and can avoid another request.
: if (isKuduMasterAction(event)) {
: return true;
I seem to recall tooling also setting this. Does that break any invariants,
e.g. if we're running the fix tool while the Kudu master isn't yet HMS-ready?
--
To view, visit http://gerrit.cloudera.org:8080/16388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37
Gerrit-Change-Number: 16388
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 11 Sep 2020 18:24:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 7: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16388/4/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/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@168 PS4, Line 168: nmentContext == null ? null : > The context should be included in the larger stack trace. OK, good luck with that :) -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 11 Sep 2020 18:18:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#7). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 229 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/7 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#6). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 213 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/6 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#5). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 189 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/5 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/16388 )
Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization
is enabled
..
Patch Set 4:
(8 comments)
I am still fixing the C++ tests.
http://gerrit.cloudera.org:8080/#/c/16388/4/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/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@131
PS4, Line 131: below doesn't fail.
> nit: If the master addresses property isn't set, won't kuduSyncEnabled() ju
Done
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@151
PS4, Line 151: if (skipsValidation()) {
: return;
: }
> nit: does it make sense to move this before tableEvent.getTable() call?
Done
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@165
PS4, Line 165: isKuduTable
> Why not to perform checkNoKuduProperties() here as well? Could you add a c
This is onDropTable, we don't care about existing properties given the table is
being dropped.
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@166
PS4, Line 166: However, make sure it doesn't have a table id from the context.
> Could you add more color on why this is a error condition which needs to be
Done
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@168
PS4, Line 168: Kudu table ID does not match the non-Kudu HMS entry
> Does it make sense to include table ID (and, maybe, table name from 'table'
The context should be included in the larger stack trace.
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@179
PS4, Line 179: // Dropping tables from Hive is okay and Kudu will be notified
via the
: // notification listener.
> nit: I didn't understand the context of this comment until I read the gerri
Done
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@181
PS4, Line 181: if (targetTableId == null) {
: return;
: }
> Should this check precede the check at line 165?
No it shouldn't. This would be a Kudu table without a targetTableId set.
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@463
PS4, Line 463: String masterAddresses =
table.getParameters().get(KUDU_MASTER_ADDRS_KEY);
: if (masterAddresses == null || masterAddresses.isEmpty()) {
: // A table without master addresses is not synchronized,
: // it may not even be a Kudu table.
: return false;
: }
> nit: could this be separated into a method/function and used here and in ch
It doesn't save any code given the behavior when unset/missing is different.
--
To view, visit http://gerrit.cloudera.org:8080/16388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37
Gerrit-Change-Number: 16388
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 10 Sep 2020 14:56:29 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16388 )
Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization
is enabled
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16388/4/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/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@151
PS4, Line 151: if (skipsValidation()) {
: return;
: }
nit: does it make sense to move this before tableEvent.getTable() call?
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@165
PS4, Line 165: isKuduTable
Why not to perform checkNoKuduProperties() here as well? Could you add a
comment clarifying on this?
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@166
PS4, Line 166: However, make sure it doesn't have a table id from the context.
Could you add more color on why this is a error condition which needs to be
handled in this handler?
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@168
PS4, Line 168: Kudu table ID does not match the non-Kudu HMS entry
Does it make sense to include table ID (and, maybe, table name from 'table'?)
into the exception message for easier troubleshooting?
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@181
PS4, Line 181: if (targetTableId == null) {
: return;
: }
Should this check precede the check at line 165?
http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@463
PS4, Line 463: String masterAddresses =
table.getParameters().get(KUDU_MASTER_ADDRS_KEY);
: if (masterAddresses == null || masterAddresses.isEmpty()) {
: // A table without master addresses is not synchronized,
: // it may not even be a Kudu table.
: return false;
: }
nit: could this be separated into a method/function and used here and in
checkMasterAddrsProperty()?
--
To view, visit http://gerrit.cloudera.org:8080/16388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37
Gerrit-Change-Number: 16388
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 09 Sep 2020 23:25:03 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 4: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/4/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/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@131 PS4, Line 131: below doesn't fail. nit: If the master addresses property isn't set, won't kuduSyncEnabled() just return false rather than fail? Nevermind. Looking around, if there is no master addresses when creating a table, we should be throwing an exception, thereby signaling to the HMS that the action is rejected. kuduSyncEnabled() is necessarily permissive because in most other cases we expect that a missing masters field refers to a non-Kudu or non-synchronized table. Could you update this to clarify that the failure isn't an exception, but rather being too permissive? http://gerrit.cloudera.org:8080/#/c/16388/4/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@179 PS4, Line 179: // Dropping tables from Hive is okay and Kudu will be notified via the : // notification listener. nit: I didn't understand the context of this comment until I read the gerrit discussion that spurred it. Could you add a bit more detail, e.g. "Drop table requests that don't come from the Kudu master may not set the table ID, e.g. when dropping tables via Hive, or dropping orphaned HMS entries. Such tables are dropped in Kudu by name via the notification listener" -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Sep 2020 18:29:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#4). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java 3 files changed, 146 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/4 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#3). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java 3 files changed, 145 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/3 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
