[KYUUBI #1816] Implement KyuubiHistoryServerPlugin #1820

wForget · 2022-01-21T09:49:32Z

Why are the changes needed?

Implement KyuubiHistoryServerPlugin. #1816

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
SparkUI:

SparkHistoryServer:

Run test locally before make a pull request

…sed for KvStore

…tTrackingStore.

# Conflicts: # externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala

codecov-commenter · 2022-01-26T11:17:01Z

Codecov Report

Merging #1820 (0aecc30) into master (53d59a0) will increase coverage by 0.09%.
The diff coverage is 52.23%.

❗ Current head 0aecc30 differs from pull request most recent head 256363d. Consider uploading reports for the commit 256363d to get more accurate results

@@             Coverage Diff              @@
##             master    #1820      +/-   ##
============================================
+ Coverage     59.46%   59.56%   +0.09%     
+ Complexity      272      268       -4     
============================================
  Files           277      284       +7     
  Lines         13853    14027     +174     
  Branches       1776     1784       +8     
============================================
+ Hits           8238     8355     +117     
- Misses         4911     4959      +48     
- Partials        704      713       +9

Impacted Files	Coverage Δ
...g/apache/kyuubi/engine/spark/KyuubiSparkUtil.scala	`88.46% <ø> (ø)`
...rg/apache/spark/ui/KyuubiHistoryServerPlugin.scala	`0.00% <0.00%> (ø)`
.../scala/org/apache/spark/ui/EngineSessionPage.scala	`2.89% <5.26%> (-0.18%)`	⬇️
...che/spark/kyuubi/SparkSQLEngineEventListener.scala	`40.62% <40.62%> (ø)`
...ache/kyuubi/engine/spark/events/SessionEvent.scala	`79.16% <50.00%> (-7.79%)`	⬇️
...uubi/engine/spark/events/SparkOperationEvent.scala	`86.48% <50.00%> (-2.41%)`	⬇️
...src/main/scala/org/apache/spark/ui/EngineTab.scala	`70.73% <63.15%> (-1.69%)`	⬇️
...kyuubi/engine/spark/events/EngineEventsStore.scala	`80.00% <77.77%> (-6.67%)`	⬇️
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	`68.53% <92.30%> (+2.27%)`	⬆️
...a/org/apache/spark/kyuubi/SparkContextHelper.scala	`64.70% <100.00%> (+4.70%)`	⬆️
... and 29 more

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 53d59a0...256363d. Read the comment docs.

wForget · 2022-01-26T11:32:34Z

Hi @yaooqinn , this implementation seems feasible. Can you help me review it?

yaooqinn · 2022-01-26T11:46:35Z

thanks, @wForget for the great job, will take a look tomorrow

yaooqinn · 2022-01-26T11:47:16Z

also cc @turboFei @ulysses-you

yaooqinn · 2022-02-08T07:12:24Z

externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala

+
+  def endTime(): Long = engine.map(_ => System.currentTimeMillis()).getOrElse {
+    sparkUI
+      .map(ui => ui.store.applicationInfo().attempts.head.endTime.getTime)


why the start/end Time always use the first attempt？

I see that the spark source code also uses attempts.head to get attempt.
like: https://github.com/apache/spark/blob/2a9416abd5c8d83901c09a08433bf91e649dc43f/core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala#L277

yaooqinn · 2022-02-08T07:18:29Z

...bi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkSQLEngineEventListener.scala

+   * The number of SQL client sessions kept in the Kyuubi Query Engine web UI.
+   */
+  private val retainedSessions: Int = {
+    sparkConf.getOption(SQLConf.THRIFTSERVER_UI_SESSION_LIMIT.key) match {


shall we simply use kyuubiConf？

yaooqinn · 2022-02-08T07:23:36Z

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/KyuubiHistoryServerPlugin.scala

+
+class KyuubiHistoryServerPlugin extends AppHistoryServerPlugin {
+
+  def kyuubiConf: KyuubiConf = KyuubiConf()


yaooqinn · 2022-02-08T07:24:52Z

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/KyuubiHistoryServerPlugin.scala

+
+class KyuubiHistoryServerPlugin extends AppHistoryServerPlugin {
+
+  def kyuubiConf: KyuubiConf = KyuubiConf()


shall we also load kyuubi configurations from sparkConf like SparkSQLEngine?

Sounds good, I will load the kyuubi configurations from sparkConf and only use kyuubiConf.

yaooqinn · 2022-02-08T07:27:02Z

Hi @yaooqinn , this implementation seems feasible. Can you help me review it?

sorry for the late reply, plans were disrupted by omicron in HZ.

ulysses-you · 2022-02-09T03:36:37Z

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/KyuubiHistoryServerPlugin.scala

+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.engine.spark.events.EngineEventsStore
+
+class KyuubiHistoryServerPlugin extends AppHistoryServerPlugin {


Can we add some comments about how to use this class ? It should work with history server together.

ulysses-you · 2022-02-09T03:39:12Z

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/KyuubiHistoryServerPlugin.scala

+  }
+
+  override def setupUI(ui: SparkUI): Unit = {
+    addKyuubiConfFromSpark(ui.conf)


it looks like we should build kyuubi conf for every Spark app UI instead of use a global kyuubi conf.

it looks like we should build kyuubi conf for every Spark app UI instead of use a global kyuubi conf.

These sparkConf seem to be the same object, should we create multiple kyuubiConf?

If we need to get the configuration of different spark app, we can get it using org.apache.spark.status.AppStatusStore#environmentInfo, do we need to do that?

These sparkConf seem to be the same object, should we create multiple kyuubiConf?

I just check the Spark build-in plugin, do not see a global value. The code is confused if the input conf is always same. It's better to craete multiple kyuubiConf.

If we need to get the configuration of different spark app, we can get it using org.apache.spark.status.AppStatusStore#environmentInfo, do we need to do that?

It seems trivial, we dont need to update.

ulysses-you · 2022-02-09T03:41:28Z

externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineSessionPage.scala

-    </ul>
-  }
+  private def generateBasicStats(): Seq[Node] =
+    if (parent.engine != null) {


the type of parent.engine is Option ?

ulysses-you · 2022-02-10T01:37:59Z

thanks, merging to master

theoctober19th · 2024-05-14T08:37:56Z

Hi, do we have a documentation for how to use this extension yet?

wForget · 2024-05-14T09:44:00Z

Hi, do we have a documentation for how to use this extension yet?

There is usage in comment of KyuubiHistoryServerPlugin.scala

kyuubi/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/KyuubiHistoryServerPlugin.scala

Lines 29 to 34 in c8e6457

    
           /** 
        
            * HistoryServer plugin for Kyuubi, It can be used as a plugin in SparkHistoryServer to make SparkHistoryServer UI display Kyuubi's Tab. 
        
            * We can use it like: 
        
            *  - Copy the kyuubi-spark-sql-engine jar to $SPARK_HOME/jars and restart SparkHistoryServer. 
        
            *  - In addition, we can add kyuubi configurations to spark-defaults.conf prefixed with "spark.kyuubi.". 
        
            */

wForget · 2024-05-14T09:46:06Z

Just copy kyuubi-spark-sql-engine.jar to $SPARK_HOME/jars of SparkHistoryServer.

wForget added 4 commits January 21, 2022 14:32

[KYUUBI-1816] Add the @KVIndex property to the event, which will be u…

fcc1a92

…sed for KvStore

[KYUUBI-1816] Add SparkSQLEngineEventListener

678396d

[KYUUBI-1816] Refactor EngineEventsStore to get Events through Elemen…

8f5b119

…tTrackingStore.

[KYUUBI-1816] Add KyuubiHistoryServerPlugin

0adf363

wForget marked this pull request as draft January 21, 2022 09:52

wForget added 2 commits January 26, 2022 15:33

[KYUUBI-1816] add AppHistoryServerPlugin services

c21f8b1

fix checkstyle

18c1845

wForget marked this pull request as ready for review January 26, 2022 07:34

wForget added 3 commits January 26, 2022 15:41

Merge remote-tracking branch 'origin/master' into KYUUBI-1816

d4f5825

# Conflicts: # externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala

fix

2481721

fix tests

86608fa

wForget changed the title ~~[WIP][KYUUBI #1816] Implement KyuubiHistoryServerPlugin~~ [KYUUBI #1816] Implement KyuubiHistoryServerPlugin Jan 26, 2022

yaooqinn requested a review from ulysses-you January 26, 2022 11:46

yaooqinn reviewed Feb 8, 2022

View reviewed changes

yaooqinn added this to the v1.5.0 milestone Feb 9, 2022

yaooqinn added the kind:feature Feature request label Feb 9, 2022

yaooqinn assigned wForget Feb 9, 2022

load kyuubiConf from sparkConf

0aecc30

ulysses-you reviewed Feb 9, 2022

View reviewed changes

yaooqinn approved these changes Feb 9, 2022

View reviewed changes

wForget added 2 commits February 9, 2022 15:31

fix

256363d

fix test

a325692

wForget requested a review from ulysses-you February 9, 2022 10:39

ulysses-you approved these changes Feb 9, 2022

View reviewed changes

ulysses-you closed this in be6133a Feb 10, 2022


		class KyuubiHistoryServerPlugin extends AppHistoryServerPlugin {

		def kyuubiConf: KyuubiConf = KyuubiConf()

[KYUUBI #1816] Implement KyuubiHistoryServerPlugin #1820

[KYUUBI #1816] Implement KyuubiHistoryServerPlugin #1820

Uh oh!

Conversation

wForget commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

codecov-commenter commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wForget commented Jan 26, 2022

Uh oh!

yaooqinn commented Jan 26, 2022

Uh oh!

yaooqinn commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Feb 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulysses-you commented Feb 10, 2022

Uh oh!

theoctober19th commented May 14, 2024

Uh oh!

wForget commented May 14, 2024

Uh oh!

wForget commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wForget commented Jan 21, 2022 •

edited

Loading

codecov-commenter commented Jan 26, 2022 •

edited

Loading