Skip to content

Conversation

@wForget
Copy link
Member

@wForget wForget commented Apr 26, 2022

Why are the changes needed?

close #2484

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
    image

  • Run test locally before make a pull request

username: String,
ip: String,
serverIp: String,
conf: Map[String, String],
Copy link
Member

Choose a reason for hiding this comment

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

I think this is connection initializing config map, how about the configs from set command

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is connection initializing config map, how about the configs from set command

The set statement will be displayed in the statement list, we don't need to override it here, it might be better to show the initial configurations.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, can't agree more. Comment on purpose 😁

Session created at {formatDate(sessionStat.startTime)},
Total run {sessionStat.totalOperations} SQL
</h4> ++
sessionPropertiesTable ++
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to be shown after SQL stats?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it better to be shown after SQL stats?

I think it would be better to put this in front of SQL stats as part of the details of the session.

Copy link
Member

Choose a reason for hiding this comment

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

OK, let keep it before SQL stats

Copy link
Member

@gabry-lab gabry-lab left a comment

Choose a reason for hiding this comment

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

need more unit tests

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022

Codecov Report

Merging #2485 (5c3bb30) into master (6187321) will decrease coverage by 0.07%.
The diff coverage is 7.69%.

❗ Current head 5c3bb30 differs from pull request most recent head 88a0282. Consider uploading reports for the commit 88a0282 to get more accurate results

@@             Coverage Diff              @@
##             master    #2485      +/-   ##
============================================
- Coverage     63.45%   63.37%   -0.08%     
  Complexity       69       69              
============================================
  Files           371      371              
  Lines         17585    17608      +23     
  Branches       2348     2348              
============================================
+ Hits          11158    11159       +1     
- Misses         5392     5416      +24     
+ Partials       1035     1033       -2     
Impacted Files Coverage Δ
.../scala/org/apache/spark/ui/EngineSessionPage.scala 2.15% <0.00%> (-0.75%) ⬇️
...ache/kyuubi/engine/spark/events/SessionEvent.scala 84.00% <100.00%> (+1.39%) ⬆️
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala 77.38% <0.00%> (+0.77%) ⬆️

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 6187321...88a0282. Read the comment docs.

@wForget
Copy link
Member Author

wForget commented Apr 26, 2022

need more unit tests

Thanks, I'll add it later

@turboFei
Copy link
Member

nice feature, thanks

Copy link
Member

@gabry-lab gabry-lab left a comment

Choose a reason for hiding this comment

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

🐂

Copy link
Contributor

@cfmcgrady cfmcgrady 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 turboFei closed this in 06da8cf Apr 27, 2022
turboFei pushed a commit that referenced this pull request Apr 27, 2022
…onPage

### _Why are the changes needed?_

close #2484

### _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
![image](https://user-images.githubusercontent.com/17894939/165425007-10a8a98e-4498-4982-b0e3-2124b976aa64.png)

- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #2485 from wForget/KYUUBI-2484.

Closes #2484

b527cae [wforget] sorted
88a0282 [wforget] add test
5c3bb30 [wforget] [KYUUBI-2484] Add conf to SessionEvent and display it in EngineSessionPage

Authored-by: wforget <643348094@qq.com>
Signed-off-by: Fei Wang <fwang12@ebay.com>
(cherry picked from commit 06da8cf)
Signed-off-by: Fei Wang <fwang12@ebay.com>
@turboFei
Copy link
Member

thanks, merged to master and branch-1.5

@yaooqinn yaooqinn added this to the 1.5.2 milestone Apr 27, 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.

[FEATURE] show session level runtime config for SparkSql engine

7 participants