Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KYUUBI #2008] Support engine type and subdomain in kyuubi-ctl #2009

Closed
wants to merge 4 commits into from

Conversation

cxzl25
Copy link
Contributor

@cxzl25 cxzl25 commented Mar 4, 2022

Why are the changes needed?

Introduced engine type and subdomain, kyuubi-ctl does not support managing engine.
Add two engine-related parameters(--engine-type --engine-subdomain).

#2008

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

@cxzl25
Copy link
Contributor Author

cxzl25 commented Mar 4, 2022

Current

image

Fix

image

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 70 to 74
// scalastyle:off println
println(logAppender.loggingEvents)
println("------")
println(searchString)
// scalastyle:on println
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we remove those debug println?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry , I added it when I improved ut locally and forgot to remove it.

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2022

Codecov Report

Merging #2009 (213a8d9) into master (d8ec325) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2009      +/-   ##
============================================
- Coverage     60.57%   60.56%   -0.01%     
  Complexity       69       69              
============================================
  Files           305      305              
  Lines         14878    14900      +22     
  Branches       1925     1927       +2     
============================================
+ Hits           9012     9024      +12     
- Misses         5096     5105       +9     
- Partials        770      771       +1     
Impacted Files Coverage Δ
...cala/org/apache/kyuubi/ctl/ServiceControlCli.scala 77.57% <100.00%> (+1.81%) ⬆️
...apache/kyuubi/ctl/ServiceControlCliArguments.scala 79.42% <100.00%> (+1.51%) ⬆️
.../kyuubi/ctl/ServiceControlCliArgumentsParser.scala 92.85% <100.00%> (+1.19%) ⬆️
...org/apache/kyuubi/operation/ExecuteStatement.scala 80.89% <0.00%> (-6.75%) ⬇️
.../org/apache/kyuubi/operation/KyuubiOperation.scala 62.66% <0.00%> (-2.67%) ⬇️
...ache/kyuubi/operation/KyuubiOperationManager.scala 92.50% <0.00%> (-2.50%) ⬇️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala 83.48% <0.00%> (-0.92%) ⬇️
...ala/org/apache/kyuubi/session/SessionManager.scala 85.12% <0.00%> (-0.83%) ⬇️
...mon/src/main/scala/org/apache/kyuubi/Logging.scala 41.09% <0.00%> (+1.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8ec325...213a8d9. Read the comment docs.

Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

LGTM. @yaooqinn Please help to merge.

@ulysses-you
Copy link
Contributor

thanks, merging to master

@ulysses-you ulysses-you added this to the v1.5.0 milestone Mar 4, 2022
@ulysses-you ulysses-you added the kind:feature Feature request label Mar 4, 2022
wForget pushed a commit to wForget/kyuubi that referenced this pull request Mar 11, 2022
### _Why are the changes needed?_
Introduced engine type and subdomain, kyuubi-ctl does not support managing engine.
Add two engine-related parameters(--engine-type --engine-subdomain).

apache#2008

### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [x] 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

Closes apache#2009 from cxzl25/KYUUBI-2008.

Closes apache#2008

213a8d9 [sychen] remove println
c3895b2 [sychen] fix style
fa1e899 [sychen] fix ut
302ae75 [sychen] support engine type and subdomain

Authored-by: sychen <sychen@trip.com>
Signed-off-by: ulysses-you <ulyssesyou@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature Feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants