[KYUUBI #1022] Add basic EngineStatusStore for events #1023

charlesy6 · 2021-09-03T12:44:29Z

Why are the changes needed?

For more detail, please go to #981

EngineStatusStore helps to push events to listener bus

EngineStatusStore is a memory store that tracking the number of statements and sessions, it provides:

stores all elements, and sorted by startTimestamp.
cleanup the last elements when reach a certain threshold.

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

charlesy6 · 2021-09-03T12:53:47Z

cc @yaooqinn, realized a single memory store called EngineStatusStore.

It's ok to rework with ElementTrackingStore. #981 (comment)

cfmcgrady · 2021-09-03T15:18:04Z

...ubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EventManager.scala

+    postListenerEvent(KyuubiEngineOperationClosedEvent(id, System.currentTimeMillis()))
+  }
+}
+


Does EventLoggingService need to store these events? @yaooqinn @ulysses-you

Maybe we make EventLoggingService as a sparklistener is better.

cc @yaooqinn @ulysses-you @cfmcgrady, looking forward to your views.

...ubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EventManager.scala

charlesy6 · 2021-09-04T03:04:09Z

Thanks @cfmcgrady @yaooqinn, make the current patch to draft.

From my point of view, the EngineStatusStore can meet the needs, but from a long-term perspective, prefer to use ElementTrackingStore. So starting rework this patch by using ElementTrackingStore.

codecov-commenter · 2021-09-06T06:08:49Z

Codecov Report

Merging #1023 (74872d7) into master (cbe5bee) will decrease coverage by 0.03%.
The diff coverage is 86.66%.

❗ Current head 74872d7 differs from pull request most recent head b9f355c. Consider uploading reports for the commit b9f355c to get more accurate results

@@             Coverage Diff              @@
##             master    #1023      +/-   ##
============================================
- Coverage     79.24%   79.21%   -0.04%     
  Complexity       90       90              
============================================
  Files           177      178       +1     
  Lines          6620     6648      +28     
  Branches        783      785       +2     
============================================
+ Hits           5246     5266      +20     
- Misses          920      926       +6     
- Partials        454      456       +2

Impacted Files	Coverage Δ
...kyuubi/engine/spark/events/EngineEventsStore.scala	`81.25% <81.25%> (ø)`
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`95.24% <83.33%> (-0.14%)`	⬇️
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	`68.00% <100.00%> (ø)`
...g/apache/spark/kyuubi/SparkSQLEngineListener.scala	`87.93% <100.00%> (+1.39%)`	⬆️
.../org/apache/kyuubi/operation/KyuubiOperation.scala	`50.00% <0.00%> (-5.36%)`	⬇️
...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala	`80.39% <0.00%> (-0.99%)`	⬇️

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 cbe5bee...b9f355c. Read the comment docs.

yaooqinn · 2021-09-06T07:59:07Z

How about we reuse the current event logging service and only introduce the ElementTrackingStore or something else as an in-memory implementation?

charlesy6 · 2021-09-06T08:16:23Z

How about we reuse the current event logging service

It's not a right way to reuse event logging service.

We should push events to SparkListenerBus, if not, the spark history server will not receive these custom events.
It's better to let different listeners do their own independent things

and only introduce the ElementTrackingStore or something else as an in-memory implementation?

In fact, using ElementTrackingStore is the least expensive, if we create something as an in-memory implementation will spend more time. I had tried these two ways.

yaooqinn · 2021-09-06T08:19:18Z

We should push events to SparkListenerBus, if not, the spark history server will not receive these custom events.

can we check SparkHistoryEventLogger?

cfmcgrady · 2021-09-06T08:56:27Z

How about we reuse the current event logging service and only introduce the ElementTrackingStore or something else as an in-memory implementation?

+1

charlesy6 · 2021-09-06T09:10:10Z

hi @yaooqinn, I can not understand why kyuubi need SparkHistoryEventLogger? it seems duplicated to spark.eventLog.enabled=true.

And, the current design of logger service is not clear to me. As we know a service means that it should listen to some port and can start / stop.

I think the following will be more clear.

charlesy6 · 2021-09-06T09:33:14Z

Looked into the logger service, the loggers are executed sequentially, and the loggers are called synchronously.

And checked the LiveListenerBus, it is an asynchronous listener bus for Spark events. SPARK-20863

yaooqinn · 2021-09-06T09:35:42Z

hi @yaooqinn, I can not understand why kyuubi need SparkHistoryEventLogger?

SparkHistoryEventLogger is used to log all Kyuubi's events to the same file of spark's event log. We also support other destinations, like Kyuubi's own JSON log store, JDBC(planned), etc..

it seems duplicated to spark.eventLog.enabled=true.

It's not. it follows spark.eventLog.enabled when logging. We just don't push the events to the listener bus but call the event log listener directly.

And, the current design of logger service is not clear to me. As we know a service means that it should listen to some port and can start / stop.

Hmm... do not go too far with the spark's listeners and the listener bus if you are not understanding what you are doing here.

IIUC, you want everything to work with spark's listeners and the listener bus, this is not what we want. We are a Spark application BUT not only a spark application that is limited to spark's functionalities.

What we want is only a size-limit buffer (using ElementTrackingStore is just an option) that holds all the current Kyuubi events in memory. Then render one or more Spark UI pages/tables based on this buffer.

yaooqinn · 2021-09-06T09:43:29Z

Looked into the logger service, the loggers are executed sequentially, and the loggers are called synchronously.

And checked the LiveListenerBus, it is an asynchronous listener bus for Spark events. SPARK-20863

Yes, this is a spot that we can improve too

charlesy6 · 2021-09-06T10:03:55Z

Yes, this is a spot that we can improve too

hi @yaooqinn, there seems to be no good way to solve the synchronization problem. For EventLoggerType.SPARK and EventLoggerType.JSON, we need to think out a asynchronous writer for local path or hdfs path.

It seems we should improve it first, because if we listener these events in this pr, it may affects the performance.

yaooqinn · 2021-09-06T10:23:53Z

Can we start with session Events only in another PR, which can minimize the review burdern

yaooqinn · 2021-09-06T10:33:11Z

We can combine the history logging and the live data logging with the listener bus but we can still use the event logging service

...nals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkContextHelper.scala

...-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/GetFunctions.scala

yaooqinn · 2021-09-06T10:49:34Z

Hmm... do not go too far with the spark's listeners and the listener bus if you are not understanding what you are doing here.

I take my early judgment back, and the listener bus looks the right way to go. #1023 (comment)

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

charlesy6 · 2021-09-06T11:01:44Z

Can we start with session Events only in another PR, which can minimize the review burdern

Sure, will do.

yaooqinn · 2021-09-06T11:03:11Z

there seems to be no good way to solve the synchronization problem.
For EventLoggerType.SPARK

This looks simple, Please send a separate PR to solve this issue

-    sc.eventLogger.foreach(_.onOtherEvent(kyuubiEvent))
+    sc.listenerBus.post(kyuubiEvent)

and EventLoggerType.JSON, we need to think out a asynchronous writer for local path or hdfs

we can implement this latter too

### _Why are the changes needed?_  > there seems to be no good way to solve the synchronization problem. For `EventLoggerType.SPARK` This looks simple, Please send a separate PR to solve this issue ```git - sc.eventLogger.foreach(_.onOtherEvent(kyuubiEvent)) + sc.listenerBus.post(kyuubiEvent) ``` _Originally posted by yaooqinn in #1023 (comment) ### _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](https://kyuubi.readthedocs.io/en/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #1044 from timothy65535/1043. Closes #1043 0ea1d4f [timothy65535] [KYUUBI #1043] Let spark history logger handle events asynchronously Authored-by: timothy65535 <timothy65535@163.com> Signed-off-by: ulysses-you <ulyssesyou18@gmail.com>

charlesy6 · 2021-09-07T08:01:01Z

We can combine the history logging and the live data logging with the listener bus but we can still use the event logging service

Had tried serveal times, but failed.

For EventLoggerType.JSON logger, it requires all fields are set.
But for EventLoggerType.SPARK logger, it don't needs all fields.

spark origin event log

{"Event":"SparkListenerJobEnd","Job ID":2,"Completion Time":1630997161352,"Job Result":{"Result":"JobSucceeded"}}
{"Event":"org.apache.spark.sql.execution.ui.SparkListenerSQLExecutionEnd","executionId":3,"time":1630997161355}
{"Event":"org.apache.spark.sql.hive.thriftserver.ui.SparkListenerThriftServerOperationFinish","id":"8f2c5e68-c9fe-4777-bffe-75d171fa18cb","finishTime":1630997161355}
{"Event":"org.apache.spark.sql.hive.thriftserver.ui.SparkListenerThriftServerOperationClosed","id":"8f2c5e68-c9fe-4777-bffe-75d171fa18cb","closeTime":1630997161374}
{"Event":"org.apache.spark.sql.hive.thriftserver.ui.SparkListenerThriftServerSessionClosed","sessionId":"7e3f25d8-5688-4170-8532-5ddee4112991","finishTime":1630997192176}
{"Event":"SparkListenerApplicationEnd","Timestamp":1630997194335}

charlesy6 · 2021-09-07T10:00:23Z

hi @yaooqinn @cfmcgrady, looking forward to your suggestions

Had read the whole ui design of sparkthriftserver
https://github.com/apache/spark/tree/master/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui

And already tried to implement a memory status store
https://github.com/timothy65535/kyuubi/blob/ky-981-t1/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/ui/EngineStatusStore.scala

At last, in terms of design compatibility and simplicity, I think it's better to reuse the thriftserver's ui design. We need to consider not only the pages on engine server, but also the pages on spark history server.

charlesy6 · 2021-09-08T02:08:53Z

cc @yaooqinn, the patch updated with customized store.

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EventLoggingService.scala

...ls/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/EngineStatusListener.scala

charlesy6 · 2021-09-09T10:20:24Z

cc @yaooqinn, already to go.

yaooqinn · 2021-09-09T10:35:58Z

can you update the PR description and maybe also some comments in the code changes to help us review?

charlesy6 · 2021-09-09T12:11:52Z

can you update the PR description and maybe also some comments in the code changes to help us review?

Updated. The store based on TreeBasedStore, it supports row keys and column keys are ordered by natural ordering by default.

charlesy6 · 2021-09-10T01:37:47Z

cc @cfmcgrady when you are free, thanks.

yaooqinn · 2021-09-10T06:36:56Z

cc @zhang1002

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

charlesy6 · 2021-09-12T06:29:35Z

cc @pan3793 @ulysses-you, help to review when free, thanks.

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala

charlesy6 · 2021-09-15T06:31:38Z

cc @yaooqinn ready to review, looking for your new advice.

yaooqinn · 2021-09-15T06:32:34Z

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala

+  /**
+   * cleanup the session events if reach the threshold
+   */
+  private def checkSessionCapacity(): Unit = {


how efficient is this？

Can't find a suitable opensource library to supports following characteristic:

Concurrency: concurrent read、update、remove

Update

Order

Complexity

If we implement a new Map which extends AbstractMap, it seems will be complex.

If we keep 200 events in memory, efficiency may not be affected.

yaooqinn · 2021-09-15T06:33:38Z

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala

+  private def checkSessionCapacity(): Unit = {
+    var countToDelete = sessions.size - retainedSessions
+
+    val reverseSeq = sessions.values().asScala.toSeq.sortBy(_.startTime).reverse


we sort the value set for evevy single event？...

if we use treemap, and let startTime or endTime as key, it will remove events if key repeat.

here you should use asyn, by this you can sort the value only once when countToDelete reached the set value.
if (retainedSessions/sessions.size >= threshold) { new thread { sort() delete() } }
also you can see Guava Cache, you can set the expire strategy by youself but it will waste some Mem...

if we use a new thread, thinghs will be more complex

charlesy6 · 2021-09-17T12:12:11Z

cc @ulysses-you @pan3793 @cfmcgrady @yaooqinn

had already tried serval implements:
V1: custom store based on treemap
V2: use spark ElementTrackingStore
V3: custom store based on TreebasedTable, ordered by startTime
V4: custom store based on ConcurrentHashMap, ordered by finishTime

looking for advice, thanks

charlesy6 · 2021-09-18T00:44:02Z

BTW, at this stage, seems we don't need to spend a lot of time on details, especially on how to implement the store, it already cost two week.

charlesy6 · 2021-09-24T03:31:36Z

cc @ulysses-you @pan3793 @cfmcgrady @yaooqinn, any more thought?

charlesy6 · 2021-09-26T01:05:25Z

cc @ulysses-you @pan3793 @yaooqinn, any more thought? thanks

yaooqinn · 2021-09-26T02:42:29Z

I am +0 on this, but since no one has an opposite option on this implementation, I get this merged.

charlesy6 · 2021-09-26T05:48:44Z

I am +0 on this, but since no one has an opposite option on this implementation

OK, new idea to the implement of the store is welcome.

cfmcgrady reviewed Sep 3, 2021

View reviewed changes

...ubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EventManager.scala Outdated Show resolved Hide resolved

charlesy6 marked this pull request as draft September 4, 2021 03:04

charlesy6 marked this pull request as ready for review September 6, 2021 06:38

yaooqinn reviewed Sep 6, 2021

View reviewed changes

...nals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkContextHelper.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 6, 2021

View reviewed changes

...-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/GetFunctions.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 6, 2021

View reviewed changes

...kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/ui/EngineAppStatusStore.scala Outdated Show resolved Hide resolved

This was referenced Sep 6, 2021

Let spark history logger handle events asynchronously #1043

Closed

[KYUUBI #1043] Let spark history logger handle events asynchronously #1044

Closed

yaooqinn reviewed Sep 8, 2021

View reviewed changes

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EventLoggingService.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 8, 2021

View reviewed changes

...ls/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/EngineStatusListener.scala Outdated Show resolved Hide resolved

charlesy6 marked this pull request as draft September 8, 2021 03:38

charlesy6 marked this pull request as ready for review September 9, 2021 01:39

cfmcgrady reviewed Sep 10, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Show resolved Hide resolved

yaooqinn reviewed Sep 14, 2021

View reviewed changes

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 14, 2021

View reviewed changes

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 14, 2021

View reviewed changes

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Sep 14, 2021

View reviewed changes

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala Show resolved Hide resolved

charlesy6 marked this pull request as draft September 15, 2021 02:25

charlesy6 marked this pull request as ready for review September 15, 2021 06:29

yaooqinn reviewed Sep 15, 2021

View reviewed changes

[KYUUBI #1022] Add basic EngineStatusStore for events

b9f355c

charlesy6 changed the title ~~[KYUUBI #1022] Add EventManager with basic EngineStatusStore~~ [KYUUBI #1022] Add basic EngineStatusStore for events Sep 18, 2021

yaooqinn closed this in 22e6432 Sep 26, 2021

charlesy6 deleted the ky-1022 branch September 26, 2021 05:31

[KYUUBI #1022] Add basic EngineStatusStore for events #1023

[KYUUBI #1022] Add basic EngineStatusStore for events #1023

Uh oh!

Conversation

charlesy6 commented Sep 3, 2021 • 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!

charlesy6 commented Sep 3, 2021

Uh oh!

cfmcgrady Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

charlesy6 Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

charlesy6 Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charlesy6 commented Sep 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

charlesy6 commented Sep 6, 2021

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

cfmcgrady commented Sep 6, 2021

Uh oh!

charlesy6 commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charlesy6 commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

charlesy6 commented Sep 6, 2021

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

Uh oh!

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

Uh oh!

charlesy6 commented Sep 6, 2021

Uh oh!

yaooqinn commented Sep 6, 2021

Uh oh!

charlesy6 commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

spark origin event log

Uh oh!

charlesy6 commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charlesy6 commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charlesy6 commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaooqinn commented Sep 9, 2021

Uh oh!

charlesy6 commented Sep 9, 2021

Uh oh!

charlesy6 commented Sep 3, 2021 •

edited

Loading

charlesy6 commented Sep 4, 2021 •

edited

Loading

codecov-commenter commented Sep 6, 2021 •

edited

Loading

charlesy6 commented Sep 6, 2021 •

edited

Loading

charlesy6 commented Sep 6, 2021 •

edited

Loading

charlesy6 commented Sep 7, 2021 •

edited

Loading

charlesy6 commented Sep 7, 2021 •

edited

Loading

charlesy6 commented Sep 8, 2021 •

edited

Loading

charlesy6 commented Sep 9, 2021 •

edited

Loading

yaooqinn Sep 15, 2021 •

edited

Loading

charlesy6 Sep 15, 2021 •

edited

Loading

charlesy6 commented Sep 17, 2021 •

edited

Loading

charlesy6 commented Sep 24, 2021 •

edited

Loading

charlesy6 commented Sep 26, 2021 •

edited

Loading

charlesy6 commented Sep 26, 2021 •

edited

Loading