You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/12/03 23:07:23 UTC

[GitHub] [incubator-kyuubi] pan3793 opened a new pull request, #3897: Supplying pluggable GroupProvider

pan3793 opened a new pull request, #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897

   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
   -->
   
   ### _Why are the changes needed?_
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you add a feature, you can talk about the use case of it.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   Kyuubi supports GROUP engine share level, and currently, simply delegates the group provider to Hadoop UserGroupInformation, which is not flexible enough for users who want to use other group mapping mechanisms, e.g. LDAP, JDBC.
   
   This PR supplies a pluggable plugin interface `GroupProvider` w/ and provides a built-in `HadoopGroupProvider` which has the same behavior w/ the current implementation.
   
   W/ this change, users can easily implement `LDAPGroupProvider`, `JDBCGroupProvider`, `FileGroupProvider`, `CustomGroupProvider`, etc. then the GROUP engine share level will be more powerful and flexible.
   
   The alternative option is to guide users to learn and extend the Hadoop group mapping system[1].
   
   [1] https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/GroupsMapping.html
   
   ### _How was this patch tested?_
   - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
   
   - [ ] Add screenshots for manual tests if appropriate
   
   - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#discussion_r1039468158


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1977,6 +1977,21 @@ object KyuubiConf {
       .stringConf
       .createOptional
 
+  val GROUP_PROVIDER: ConfigEntry[String] =
+    buildConf("kyuubi.session.group.provider")
+      .doc("A group provider plugin for Kyuubi Server. This plugin can provide primary group " +
+        "and groups information for different user or session configs. This config value " +
+        "should be a class which is a child of 'org.apache.kyuubi.plugin.GroupProvider' which " +
+        "has zero-arg constructor. Kyuubi provides the following built-in implementations: " +
+        "<li>hadoop: delegate the user group mapping to hadoop UserGroupInformation.</li>")
+      .version("1.7.0")
+      .stringConf
+      .transform {
+        case "hadoop" => "org.apache.kyuubi.session.HadoopGroupProvider"

Review Comment:
   We should provide the built-in implementation list in the description, and provide a shortcut like "hadoop", "ldap", "unix" for convenient, but allow users to set the full class name for third party implementation.
   I'm planning to change to the `SESSION_CONF_ADVISOR`'s documentation to align w/ this way, because it does not clear which built-in implementations are available.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#discussion_r1039278366


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1977,6 +1977,21 @@ object KyuubiConf {
       .stringConf
       .createOptional
 
+  val GROUP_PROVIDER: ConfigEntry[String] =
+    buildConf("kyuubi.session.group.provider")
+      .doc("A group provider plugin for Kyuubi Server. This plugin can provide primary group " +
+        "and groups information for different user or session configs. This config value " +
+        "should be a class which is a child of 'org.apache.kyuubi.plugin.GroupProvider' which " +
+        "has zero-arg constructor. Kyuubi provides the following built-in implementations: " +
+        "<li>hadoop: delegate the user group mapping to hadoop UserGroupInformation.</li>")
+      .version("1.7.0")
+      .stringConf
+      .transform {
+        case "hadoop" => "org.apache.kyuubi.session.HadoopGroupProvider"

Review Comment:
   Whenever a built -in implementation is added, you need to change the code here and re -generate the document. It seems that it is not very friendly to users. Can we define it like session_conf_advisor?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#issuecomment-1337178211

   Thanks all for review, merging to master


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 closed pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3897: Supplying pluggable GroupProvider
URL: https://github.com/apache/incubator-kyuubi/pull/3897


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#discussion_r1038882842


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1977,6 +1977,21 @@ object KyuubiConf {
       .stringConf
       .createOptional
 
+  val GROUP_PROVIDER: ConfigEntry[String] =
+    buildConf("kyuubi.session.group.provider")
+      .doc("A group provider plugin for Kyuubi Server. This plugin can provide primary group " +
+        "and groups information for different user or session configs. This config value " +
+        "should be a class which is a child of 'org.apache.kyuubi.plugin.GroupProvider' which " +
+        "has zero-arg constructor. Kyuubi provides the following built-in implementations: " +
+        "<li>hadoop: delegate the user group mapping to hadoop UserGroupInformation.</li>")
+      .version("1.7.0")
+      .stringConf
+      .transform {
+        case "hadoop" => "org.apache.kyuubi.session.HadoopGroupProvider"

Review Comment:
   give the built-in a shortcut



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#discussion_r1039278366


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1977,6 +1977,21 @@ object KyuubiConf {
       .stringConf
       .createOptional
 
+  val GROUP_PROVIDER: ConfigEntry[String] =
+    buildConf("kyuubi.session.group.provider")
+      .doc("A group provider plugin for Kyuubi Server. This plugin can provide primary group " +
+        "and groups information for different user or session configs. This config value " +
+        "should be a class which is a child of 'org.apache.kyuubi.plugin.GroupProvider' which " +
+        "has zero-arg constructor. Kyuubi provides the following built-in implementations: " +
+        "<li>hadoop: delegate the user group mapping to hadoop UserGroupInformation.</li>")
+      .version("1.7.0")
+      .stringConf
+      .transform {
+        case "hadoop" => "org.apache.kyuubi.session.HadoopGroupProvider"

Review Comment:
   Whenever a built -in implementation is added, you need to change the code here and re -generate the document. It seems that it is not very friendly to users. Can't you define it like session_conf_advisor?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#discussion_r1038882842


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1977,6 +1977,21 @@ object KyuubiConf {
       .stringConf
       .createOptional
 
+  val GROUP_PROVIDER: ConfigEntry[String] =
+    buildConf("kyuubi.session.group.provider")
+      .doc("A group provider plugin for Kyuubi Server. This plugin can provide primary group " +
+        "and groups information for different user or session configs. This config value " +
+        "should be a class which is a child of 'org.apache.kyuubi.plugin.GroupProvider' which " +
+        "has zero-arg constructor. Kyuubi provides the following built-in implementations: " +
+        "<li>hadoop: delegate the user group mapping to hadoop UserGroupInformation.</li>")
+      .version("1.7.0")
+      .stringConf
+      .transform {
+        case "hadoop" => "org.apache.kyuubi.session.HadoopGroupProvider"

Review Comment:
   give the built-in implementation a shortcut



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#issuecomment-1336302793

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3897](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b100348) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f1218ae3972674e123392844f7cb45a1249c5b28?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f1218ae) will **increase** coverage by `0.02%`.
   > The diff coverage is `77.14%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3897      +/-   ##
   ============================================
   + Coverage     51.79%   51.82%   +0.02%     
     Complexity       13       13              
   ============================================
     Files           506      508       +2     
     Lines         28958    28981      +23     
     Branches       3989     3991       +2     
   ============================================
   + Hits          14999    15019      +20     
   - Misses        12534    12538       +4     
   + Partials       1425     1424       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...n/java/org/apache/kyuubi/plugin/GroupProvider.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zZXJ2ZXIva3l1dWJpLXNlcnZlci1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vR3JvdXBQcm92aWRlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `73.17% <0.00%> (-0.68%)` | :arrow_down: |
   | [...g/apache/kyuubi/session/KyuubiSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaVNlc3Npb25NYW5hZ2VyLnNjYWxh) | `88.13% <0.00%> (-0.76%)` | :arrow_down: |
   | [.../org/apache/kyuubi/session/KyuubiSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaVNlc3Npb25JbXBsLnNjYWxh) | `77.16% <83.33%> (+0.74%)` | :arrow_up: |
   | [...rg/apache/kyuubi/session/HadoopGroupProvider.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0hhZG9vcEdyb3VwUHJvdmlkZXIuc2NhbGE=) | `85.71% <85.71%> (ø)` | |
   | [.../scala/org/apache/kyuubi/plugin/PluginLoader.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vUGx1Z2luTG9hZGVyLnNjYWxh) | `90.00% <88.88%> (-1.67%)` | :arrow_down: |
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `97.48% <100.00%> (+0.08%)` | :arrow_up: |
   | [...mon/src/main/scala/org/apache/kyuubi/Logging.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9Mb2dnaW5nLnNjYWxh) | `52.63% <0.00%> (-1.32%)` | :arrow_down: |
   | [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `89.27% <0.00%> (-0.70%)` | :arrow_down: |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3897/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3897: Supplying pluggable GroupProvider

Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3897:
URL: https://github.com/apache/incubator-kyuubi/pull/3897#issuecomment-1336305005

   cc @yanghua @jiaoqingbo @cxzl25


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org