Skip to content

Conversation

@ychris78
Copy link
Contributor

@ychris78 ychris78 commented Nov 1, 2022

Why are the changes needed?

When a kyuubi service connections to multiple engine clusters, we can only explicitly specify relevant parameters after the URL, for example:
beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.engineEnv.FLINK_HOME=/opt/flink;kyuubi.ha.namespace=kyuubi-ns-c;kyuubi.engine.type=FLINK_SQL'
This method is extremely unfriendly to users.

I implemented a SessionConfAdvisor implementation class org.apache.kyuubi.session.FileSessionConfAdvisor based on the conf file that to manage session level configurations, so that we can access this cluster only by specifying a unique engine name, for example:
access cluster-a:
beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.session.conf.profile=cluster-a'

access cluster-c:
beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.session.conf.profile=cluster-c'

kyuubi-session-cluster-a.conf configuration is as follows:

    kyuubi.ha.namespace kyuubi-ns-a
    kyuubi.engine.type SPARK_SQL
    kyuubi.engine.pool.balance.policy POLLING

    kyuubi.engineEnv.SPARK_HOME /opt/spark30
    kyuubi.engineEnv.HADOOP_CONF_DIR /opt/hadoop_conf_dir

kyuubi-session-cluster-c.conf configuration is as follows:

    kyuubi.ha.namespace kyuubi-ns-a
    kyuubi.engine.type SPARK_SQL

    kyuubi.engineEnv.SPARK_HOME /opt/spark32
    kyuubi.engineEnv.HADOOP_CONF_DIR /opt/hadoop_conf_dir

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

… only explicitly specify relevant parameters after the URL, for example:

beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.engineEnv.FLINK_HOME=/opt/flink;kyuubi.ha.namespace=kyuubi-ns-c;kyuubi.engine.type=FLINK_SQL'
This method is extremely unfriendly to users, and it is extremely unsafe to expose namespaces to users.
So I added a yaml file(engine-cluster-env.yaml) to manage the engine clusters, so that we can use this cluster only by specifying a unique engine name, for example:
use cluster-a:
beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.engine.cluster.name=cluster-a'

use cluster-c:
beeline -u 'jdbc:hive2://127.0.0.1:10009/?kyuubi.engine.cluster.name=cluster-c'

engine-cluster-env.yaml configuration is as follows:

cluster-a:
  conf:
    "kyuubi.ha.namespace": kyuubi-ns-a
    "kyuubi.engine.type": SPARK_SQL
    "kyuubi.engine.pool.balance.policy": POLLING
  env:
    SPARK_HOME: /opt/spark
    HADOOP_CONF_DIR: /opt/hadoop_conf_dir

cluster-b:
  conf:
    "kyuubi.ha.namespace": kyuubi-ns-b
    "kyuubi.engine.type": SPARK_SQL
    "kyuubi.engine.pool.balance.policy": POLLING
  env:
    SPARK_HOME: /opt/spark2
    HADOOP_CONF_DIR: /opt/hadoop_conf_dir2

cluster-c:
  conf:
    "kyuubi.ha.namespace": kyuubi-ns-c
    "kyuubi.engine.type": FLINK_SQL
    "kyuubi.engine.pool.balance.policy": POLLING
  env:
    FLINK_HOME: /opt/flink
@turboFei
Copy link
Member

turboFei commented Nov 1, 2022

It looks like kyuubi.session.conf.tag instead of cluster.

@pan3793
Copy link
Member

pan3793 commented Nov 1, 2022

How about leveraging SessionConfAdvisor to implement such functionality?

@pan3793
Copy link
Member

pan3793 commented Nov 1, 2022

I'm thinking of some ideas that may be related.

Currently, Kyuubi provides the SessionConfAdvisor extended point but no build-in implementation, maybe we can supply some simple but generic implementations.

For example,

  1. file-based configuration collections, defining custom session confs in a separate <profile>.conf file, the idea is similar to this PR proposed IMO;
  2. JDBC-based configuration collections, injecting session confs by user/group name stored in RDBMS

@ychris78 ychris78 changed the title add a yaml file to manage the engine clusters When a kyuubi service connections to multiple engine clusters Add a yaml file to manage session level configuration When the kyuubi service supports accessing multiple engine clusters Nov 2, 2022
@ychris78
Copy link
Contributor Author

ychris78 commented Nov 2, 2022

It looks like kyuubi.session.conf.tag instead of cluster.

Sorry for the trouble caused by the inappropriate title, which has been corrected

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #3742 (68bbfec) into master (f21ad83) will increase coverage by 0.31%.
The diff coverage is 96.15%.

❗ Current head 68bbfec differs from pull request most recent head e144986. Consider uploading reports for the commit e144986 to get more accurate results

@@             Coverage Diff              @@
##             master    #3742      +/-   ##
============================================
+ Coverage     52.57%   52.89%   +0.31%     
  Complexity       13       13              
============================================
  Files           492      497       +5     
  Lines         27705    27994     +289     
  Branches       3830     3860      +30     
============================================
+ Hits          14566    14807     +241     
- Misses        11751    11788      +37     
- Partials       1388     1399      +11     
Impacted Files Coverage Δ
...apache/kyuubi/session/FileSessionConfAdvisor.scala 94.11% <94.11%> (ø)
...ommon/src/main/scala/org/apache/kyuubi/Utils.scala 76.22% <100.00%> (+0.33%) ⬆️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 97.47% <100.00%> (-0.07%) ⬇️
...ache/kyuubi/engine/flink/FlinkProcessBuilder.scala 71.42% <100.00%> (ø)
...va/org/apache/kyuubi/client/api/v1/dto/Engine.java 60.34% <0.00%> (-11.75%) ⬇️
...apache/kyuubi/engine/JpsApplicationOperation.scala 77.41% <0.00%> (-3.23%) ⬇️
...main/scala/org/apache/kyuubi/ctl/util/Render.scala 95.18% <0.00%> (-0.66%) ⬇️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala 80.74% <0.00%> (-0.63%) ⬇️
...a/org/apache/kyuubi/service/TFrontendService.scala 90.98% <0.00%> (-0.19%) ⬇️
... and 36 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ychris78
Copy link
Contributor Author

ychris78 commented Nov 2, 2022

I'm thinking of some ideas that may be related.

Currently, Kyuubi provides the SessionConfAdvisor extended point but no build-in implementation, maybe we can supply some simple but generic implementations.

For example,

  1. file-based configuration collections, defining custom session confs in a separate <profile>.conf file, the idea is similar to this PR proposed IMO;
  2. JDBC-based configuration collections, injecting session confs by user/group name stored in RDBMS

Changed to the implementation of SessionConfAdvisor based on yaml file, please review again, thanks

When kyuubi.session.conf.advisor=org.apache.kyuubi.plugin.FileSessionConfAdvisor,Specify a session level configuration file, which will be combined with default.conf to have an impact. The corresponding configuration file is conf/kyuubi-session-<profile>.conf
@ychris78 ychris78 changed the title Add a yaml file to manage session level configuration When the kyuubi service supports accessing multiple engine clusters Add FileSessionConfAdvisor to manage session level configuration Nov 8, 2022
@pan3793 pan3793 added this to the v1.7.0 milestone Nov 10, 2022
@ychris78 ychris78 requested a review from turboFei November 14, 2022 06:43
Copy link
Member

@turboFei turboFei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@turboFei
Copy link
Member

Some confusion on the pr description:

and it is extremely unsafe to expose namespaces to users.

  1. Customer can check it on spark UI, environments tab.
  2. Customer can check it with sql command set spark.kyuubi.ha.namespace.

@ychris78
Copy link
Contributor Author

Some confusion on the pr description:

and it is extremely unsafe to expose namespaces to users.

  1. Customer can check it on spark UI, environments tab.
  2. Customer can check it with sql command set spark.kyuubi.ha.namespace.

removed

@pan3793
Copy link
Member

pan3793 commented Nov 14, 2022

Thanks @ychris78 and all reviewers, merging to master

@pan3793 pan3793 closed this in 8788c3b Nov 14, 2022
@ychris78
Copy link
Contributor Author

Thanks @ychris78 and all reviewers, merging to master

thanks, merged to master

ychris78 added a commit to ychris78/kyuubi that referenced this pull request Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants