You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/01/28 09:15:35 UTC
[GitHub] [incubator-kyuubi] iodone opened a new pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
iodone opened a new pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
Use case is simple:
```
// scribe sync
EventBus.register[Test0KyuubiEvent](new Test0Handler with EventHandler)
// scribe async
EventBus.registerAsync[Test0KyuubiEvent](new Test0Handler with EventHandler)
// send event
EventBus.post(KyuubiEvent("test0"))
EventBus.post(KyuubiEvent("test1"))
```
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] wForget commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
wForget commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033385212
> cc @wForget, can you help check this PR?
Sorry, I'm confused about this, this is overcorrect for my needs.
For me, I just need to store the event asynchronously and don't need to add some handlers to handle it when producing events. I will read the event in storage(like es), then process and audit it.
In addition, I feel this is similar to before:
1. EventBus is similar to EventLoggingService
2. EventHandler is similar to EventLogger
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1077609911
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1853](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90248f2) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/e8445b7f28920c35aa2bd8e53c545143cd902dd2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8445b7) will **decrease** coverage by `0.12%`.
> The diff coverage is `83.96%`.
> :exclamation: Current head 90248f2 differs from pull request most recent head 810cc4e. Consider uploading reports for the commit 810cc4e to get more accurate results
```diff
@@ Coverage Diff @@
## master #1853 +/- ##
============================================
- Coverage 61.81% 61.68% -0.13%
Complexity 69 69
============================================
Files 331 334 +3
Lines 16016 15964 -52
Branches 2031 2028 -3
============================================
- Hits 9900 9848 -52
- Misses 5295 5296 +1
+ Partials 821 820 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ala/org/apache/kyuubi/events/handler/package.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9wYWNrYWdlLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/spark/kyuubi/SparkContextHelper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrQ29udGV4dEhlbHBlci5zY2FsYQ==) | `52.94% <33.33%> (-4.96%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `65.25% <72.22%> (+0.76%)` | :arrow_up: |
| [.../events/handler/SparkJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0pzb25Mb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `83.33% <83.33%> (ø)` | |
| [...events/handler/ServerJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9TZXJ2ZXJKc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `83.33% <83.33%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `65.59% <87.50%> (+2.62%)` | :arrow_up: |
| [...main/scala/org/apache/kyuubi/events/EventBus.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRCdXMuc2NhbGE=) | `88.57% <88.57%> (ø)` | |
| [...yuubi/events/handler/JsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9Kc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `82.92% <90.00%> (ø)` | |
| [...ents/handler/SparkHistoryLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0hpc3RvcnlMb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `84.12% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e8445b7...810cc4e](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] wForget commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
wForget commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r802268560
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/events/EventBus.scala
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.events
+
+import scala.collection.mutable
+import scala.concurrent.{ExecutionContext, Future}
+import scala.reflect.{classTag, ClassTag}
+import scala.util.Try
+
+import org.apache.kyuubi.Logging
+import org.apache.kyuubi.util.ThreadUtils
+
+sealed trait EventBus {
+ def post[T <: KyuubiEvent](event: T): Unit
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+}
+
+object EventBus extends Logging {
+ private val defaultEventBus = EventBusLive()
+
+ def apply(): EventBus = EventBusLive()
+
+ // Exposed api
+ def post[T <: KyuubiEvent](event: T): Unit = defaultEventBus.post[T](event)
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.register[T](et)
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.registerAsync[T](et)
+
+ private case class EventBusLive() extends EventBus {
+ private[this] val eventHandlerRegistry = new Registry
+ private[this] val asyncEventHandlerRegistry = new Registry
+ implicit private[this] val ec: ExecutionContext =
+ ExecutionContext.fromExecutor(
+ ThreadUtils.newDaemonQueuedThreadPool(10, 10, 60000L, "event-bus-exec-pool"))
Review comment:
The queue size seems to be too small, and how do we handle subsequent events when the queue is full?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] wForget commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
wForget commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1080107125
> > LGTM, in addition, I have some comments:
> >
> > 1. Why don't we register handlers with `EventBus.registerAsync`?
> > 2. `SparkJsonLoggingEventHandler` and `ServerJsonLoggingEventHandler` don't seem to do anything, they seem unnecessary.
> > 3. Do we need to use a separate `ExecutionContext` for EventBus instead of `Implicits.global`?
>
> @wForget, thank you for your advice. For points 1, 3: Currently, since the Async pattern has not been validated in real scenarios:
>
> 1. Just the same implementation of the previous LoggerService is done, not upgraded to asynchronous.
> 2. Scala global `executioncontext` uses the `ForkJoin` settings related to the system, which can ensure optimal performance. We can tuning it when using async EventHandler in the future like the implement of `AsyncElasticSearchLoggingHandler`
>
> For points 2: For decoupling and extensibility, while also facilitating independent unit testing
yea, these are some small doubts. The overall implementation looks good. @yaooqinn @ulysses-you WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r802351332
##########
File path: externals/kyuubi-download/pom.xml
##########
@@ -45,6 +45,7 @@
</goals>
<configuration>
<skip>${spark.archive.download.skip}</skip>
+ <skipCache>true</skipCache>
Review comment:
A small problem found during testing, need to submit a new issue to fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r837195305
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala
##########
@@ -170,15 +169,37 @@ object SparkSQLEngine extends Logging {
kyuubiConf)
val event = EngineEvent(engine)
info(event)
- EventLogging.onEvent(event)
+ EventBus.post(event)
} catch {
case t: Throwable =>
throw new KyuubiException(s"Failed to start SparkSQLEngine: ${t.getMessage}", t)
}
// Stop engine before SparkContext stopped to avoid calling a stopped SparkContext
addShutdownHook(() => engine.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 2)
- addShutdownHook(() => eventLogging.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 1)
}
+
+ def initLoggerEventHandler(conf: KyuubiConf): Unit = {
+ conf.get(ENGINE_EVENT_LOGGERS)
+ .map(EventLoggerType.withName)
+ .foreach {
+ case EventLoggerType.SPARK =>
+ EventBus.register[KyuubiEvent](new SparkHistoryLoggingEventHandler(spark.sparkContext))
+ case EventLoggerType.JSON =>
+ val handler = SparkJsonLoggingEventHandler(
+ spark.sparkContext.applicationAttemptId.getOrElse(spark.sparkContext.applicationId),
Review comment:
Thanks, i will resolve it ASAP
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033571630
The above is the overall design of the `EventBus` implementation, there is no relationship with the `EventLoggerService`, `EventLoggerService` is seen as a specific implementation of the `EventHandler`. `EventBus` main functions:
1. Register a `KyuubiEvent` eventHandler, where the `EventHandler` can be `EventLoggerService`, `AuditService`, or any other logic for handling events
2. Trigger an event at some place
By adding `EventBus` is mainly to decouple `EventLoggerService`, I think `EventLoggerService` is only suitable to handle events, it is just an `EventHandler`, it should not trigger event, this is the responsibility of `EventBus`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033484647
The `EventLogger` s, such as `JsonEventLogger` extends `AbstractService` now which makes it hard to developers to implement.
And the EventLoggingService is now sync and we shall make it async simply.
WDYT? @wForget @iodone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r801334626
##########
File path: externals/kyuubi-download/pom.xml
##########
@@ -45,6 +45,7 @@
</goals>
<configuration>
<skip>${spark.archive.download.skip}</skip>
+ <skipCache>true</skipCache>
Review comment:
this seems irrelevant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1077609911
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1853](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (33e063c) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/88e0bcd2a1d91de2c601dd3ecf2b14f4e3ae3501?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88e0bcd) will **decrease** coverage by `1.84%`.
> The diff coverage is `88.23%`.
> :exclamation: Current head 33e063c differs from pull request most recent head 90248f2. Consider uploading reports for the commit 90248f2 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1853 +/- ##
============================================
- Coverage 61.68% 59.84% -1.85%
- Complexity 69 269 +200
============================================
Files 332 284 -48
Lines 15947 14020 -1927
Branches 2027 1785 -242
============================================
- Hits 9837 8390 -1447
+ Misses 5290 4921 -369
+ Partials 820 709 -111
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../main/scala/org/apache/kyuubi/events/package.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvcGFja2FnZS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...main/scala/org/apache/kyuubi/events/EventBus.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRCdXMuc2NhbGE=) | `89.47% <89.47%> (ø)` | |
| [...org/apache/kyuubi/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `87.64% <100.00%> (+1.61%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `64.28% <100.00%> (+1.32%)` | :arrow_up: |
| [.../org/apache/kyuubi/session/KyuubiSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaVNlc3Npb25JbXBsLnNjYWxh) | `83.33% <100.00%> (+6.94%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/events/JsonProtocol.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvSnNvblByb3RvY29sLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ala/org/apache/kyuubi/events/EventLoggerType.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRMb2dnZXJUeXBlLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...on/ZooKeeperEngineSecuritySecretProviderImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NlcnZpY2UvYXV0aGVudGljYXRpb24vWm9vS2VlcGVyRW5naW5lU2VjdXJpdHlTZWNyZXRQcm92aWRlckltcGwuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/kyuubi/ha/client/ServiceNodeInfo.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlTm9kZUluZm8uc2NhbGE=) | `0.00% <0.00%> (-87.50%)` | :arrow_down: |
| [...ice/authentication/FEServiceProcessorFactory.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL2F1dGhlbnRpY2F0aW9uL0ZFU2VydmljZVByb2Nlc3NvckZhY3Rvcnkuc2NhbGE=) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| ... and [166 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88e0bcd...90248f2](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1082526784
thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you closed pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1077609911
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1853](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (beabf05) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/e8445b7f28920c35aa2bd8e53c545143cd902dd2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8445b7) will **increase** coverage by `0.00%`.
> The diff coverage is `83.96%`.
```diff
@@ Coverage Diff @@
## master #1853 +/- ##
=========================================
Coverage 61.81% 61.81%
Complexity 69 69
=========================================
Files 331 333 +2
Lines 16016 16033 +17
Branches 2031 2032 +1
=========================================
+ Hits 9900 9911 +11
- Misses 5295 5301 +6
Partials 821 821
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ala/org/apache/kyuubi/events/handler/package.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9wYWNrYWdlLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/spark/kyuubi/SparkContextHelper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrQ29udGV4dEhlbHBlci5zY2FsYQ==) | `52.94% <33.33%> (-4.96%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `65.25% <72.22%> (+0.76%)` | :arrow_up: |
| [.../events/handler/SparkJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0pzb25Mb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `83.33% <83.33%> (ø)` | |
| [...events/handler/ServerJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9TZXJ2ZXJKc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `83.33% <83.33%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `65.59% <87.50%> (+2.62%)` | :arrow_up: |
| [...main/scala/org/apache/kyuubi/events/EventBus.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRCdXMuc2NhbGE=) | `88.57% <88.57%> (ø)` | |
| [...yuubi/events/handler/JsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9Kc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `82.92% <90.00%> (ø)` | |
| [...ents/handler/SparkHistoryLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0hpc3RvcnlMb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `84.12% <100.00%> (ø)` | |
| ... and [7 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e8445b7...beabf05](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r801337072
##########
File path: kyuubi-server/src/main/resources/kyuubi-defaults.conf
##########
@@ -0,0 +1,26 @@
+#
Review comment:
this file is unnecessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn edited a comment on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn edited a comment on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033484647
The `EventLogger` s, such as `JsonEventLogger` extends `AbstractService` now which makes it hard to developers to implement. we shall make it AbstractService-free.
And the EventLoggingService is now sync and we shall make it async simply.
WDYT? @wForget @iodone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033478961
Now, we have EventHandler/EventLogger/EventBus/EventBusLive/EventLoggingService, it's confusing. Maybe we can make the new design to keep it simple.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1078567183
@yaooqinn , Could you help me review the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1032295305
cc @wForget, can you help check this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1077609911
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1853](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (beabf05) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f80d1a8be35825bff5d2e42177ff8ea1d7daa713?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f80d1a8) will **decrease** coverage by `0.09%`.
> The diff coverage is `83.96%`.
> :exclamation: Current head beabf05 differs from pull request most recent head d2ecb42. Consider uploading reports for the commit d2ecb42 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1853 +/- ##
============================================
- Coverage 61.91% 61.81% -0.10%
Complexity 69 69
============================================
Files 331 333 +2
Lines 16051 16033 -18
Branches 2036 2032 -4
============================================
- Hits 9938 9911 -27
- Misses 5284 5301 +17
+ Partials 829 821 -8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ala/org/apache/kyuubi/events/handler/package.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9wYWNrYWdlLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/spark/kyuubi/SparkContextHelper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrQ29udGV4dEhlbHBlci5zY2FsYQ==) | `52.94% <33.33%> (-4.96%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `65.25% <72.22%> (-9.95%)` | :arrow_down: |
| [.../events/handler/SparkJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0pzb25Mb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `83.33% <83.33%> (ø)` | |
| [...events/handler/ServerJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9TZXJ2ZXJKc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `83.33% <83.33%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `65.59% <87.50%> (+2.62%)` | :arrow_up: |
| [...main/scala/org/apache/kyuubi/events/EventBus.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRCdXMuc2NhbGE=) | `88.57% <88.57%> (ø)` | |
| [...yuubi/events/handler/JsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9Kc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `82.92% <90.00%> (ø)` | |
| [...ents/handler/SparkHistoryLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0hpc3RvcnlMb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `84.12% <100.00%> (+1.77%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [dcc71b3...d2ecb42](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1056324381
> Sorry for the late reply.
>
> the idea is great, but can we make it a bit simple. As `EventLogger` can be a typical `EventHandler`, we can remove one layer(EventLoggerService) directly.
OK，i will improve it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r837045518
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala
##########
@@ -170,15 +169,37 @@ object SparkSQLEngine extends Logging {
kyuubiConf)
val event = EngineEvent(engine)
info(event)
- EventLogging.onEvent(event)
+ EventBus.post(event)
} catch {
case t: Throwable =>
throw new KyuubiException(s"Failed to start SparkSQLEngine: ${t.getMessage}", t)
}
// Stop engine before SparkContext stopped to avoid calling a stopped SparkContext
addShutdownHook(() => engine.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 2)
- addShutdownHook(() => eventLogging.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 1)
}
+
+ def initLoggerEventHandler(conf: KyuubiConf): Unit = {
+ conf.get(ENGINE_EVENT_LOGGERS)
+ .map(EventLoggerType.withName)
+ .foreach {
+ case EventLoggerType.SPARK =>
+ EventBus.register[KyuubiEvent](new SparkHistoryLoggingEventHandler(spark.sparkContext))
+ case EventLoggerType.JSON =>
+ val handler = SparkJsonLoggingEventHandler(
+ spark.sparkContext.applicationAttemptId.getOrElse(spark.sparkContext.applicationId),
Review comment:
`applicationAttemptId` is a number of attempt count, e.g. `1`
The right name maybe:
```
spark.sparkContext.applicationAttemptId
.map(id => s"${spark.sparkContext.applicationId}_$id")
.getOrElse(spark.sparkContext.applicationId)
```
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala
##########
@@ -170,15 +169,37 @@ object SparkSQLEngine extends Logging {
kyuubiConf)
val event = EngineEvent(engine)
info(event)
- EventLogging.onEvent(event)
+ EventBus.post(event)
} catch {
case t: Throwable =>
throw new KyuubiException(s"Failed to start SparkSQLEngine: ${t.getMessage}", t)
}
// Stop engine before SparkContext stopped to avoid calling a stopped SparkContext
addShutdownHook(() => engine.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 2)
- addShutdownHook(() => eventLogging.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 1)
}
+
+ def initLoggerEventHandler(conf: KyuubiConf): Unit = {
+ conf.get(ENGINE_EVENT_LOGGERS)
+ .map(EventLoggerType.withName)
+ .foreach {
+ case EventLoggerType.SPARK =>
+ EventBus.register[KyuubiEvent](new SparkHistoryLoggingEventHandler(spark.sparkContext))
+ case EventLoggerType.JSON =>
+ val handler = SparkJsonLoggingEventHandler(
+ spark.sparkContext.applicationAttemptId.getOrElse(spark.sparkContext.applicationId),
Review comment:
I see this code is coied, just fixes it together
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/events/EventBus.scala
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.events
+
+import scala.collection.mutable
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.reflect.{classTag, ClassTag}
+import scala.util.Try
+
+import org.apache.kyuubi.Logging
+import org.apache.kyuubi.events.handler.EventHandler
+
+sealed trait EventBus {
+ def post[T <: KyuubiEvent](event: T): Unit
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+}
+
+object EventBus extends Logging {
+ private val defaultEventBus = EventBusLive()
+
+ def apply(): EventBus = EventBusLive()
+
+ // Exposed api
+ def post[T <: KyuubiEvent](event: T): Unit = defaultEventBus.post[T](event)
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.register[T](et)
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.registerAsync[T](et)
+
+ private case class EventBusLive() extends EventBus {
+ private[this] val eventHandlerRegistry = new Registry
+ private[this] val asyncEventHandlerRegistry = new Registry
Review comment:
add `lazy`
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala
##########
@@ -170,15 +169,37 @@ object SparkSQLEngine extends Logging {
kyuubiConf)
val event = EngineEvent(engine)
info(event)
- EventLogging.onEvent(event)
+ EventBus.post(event)
} catch {
case t: Throwable =>
throw new KyuubiException(s"Failed to start SparkSQLEngine: ${t.getMessage}", t)
}
// Stop engine before SparkContext stopped to avoid calling a stopped SparkContext
addShutdownHook(() => engine.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 2)
- addShutdownHook(() => eventLogging.stop(), SPARK_CONTEXT_SHUTDOWN_PRIORITY + 1)
}
+
+ def initLoggerEventHandler(conf: KyuubiConf): Unit = {
+ conf.get(ENGINE_EVENT_LOGGERS)
+ .map(EventLoggerType.withName)
+ .foreach {
+ case EventLoggerType.SPARK =>
+ EventBus.register[KyuubiEvent](new SparkHistoryLoggingEventHandler(spark.sparkContext))
+ case EventLoggerType.JSON =>
+ val handler = SparkJsonLoggingEventHandler(
+ spark.sparkContext.applicationAttemptId.getOrElse(spark.sparkContext.applicationId),
+ ENGINE_EVENT_JSON_LOG_PATH,
+ spark.sparkContext.hadoopConfiguration,
+ conf)
+
+ // register JsonLogger as a event handler for default event bus
+ EventBus.register[KyuubiEvent](handler)
+ case logger =>
+ // TODO: Add more implementations
+ throw new IllegalArgumentException(s"Unrecognized event logger: $logger")
+ }
+
Review comment:
unnecessary line
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/events/EventBus.scala
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.events
+
+import scala.collection.mutable
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.reflect.{classTag, ClassTag}
+import scala.util.Try
+
+import org.apache.kyuubi.Logging
+import org.apache.kyuubi.events.handler.EventHandler
+
+sealed trait EventBus {
+ def post[T <: KyuubiEvent](event: T): Unit
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
Review comment:
why this and some other place name it `et`... how about `eventHander` ?
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/events/EventBus.scala
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.events
+
+import scala.collection.mutable
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.Future
+import scala.reflect.{classTag, ClassTag}
+import scala.util.Try
+
+import org.apache.kyuubi.Logging
+import org.apache.kyuubi.events.handler.EventHandler
+
+sealed trait EventBus {
Review comment:
can we enrich the class description ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1077609911
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1853](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90248f2) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/e8445b7f28920c35aa2bd8e53c545143cd902dd2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8445b7) will **decrease** coverage by `0.12%`.
> The diff coverage is `83.96%`.
> :exclamation: Current head 90248f2 differs from pull request most recent head beabf05. Consider uploading reports for the commit beabf05 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1853 +/- ##
============================================
- Coverage 61.81% 61.68% -0.13%
Complexity 69 69
============================================
Files 331 334 +3
Lines 16016 15964 -52
Branches 2031 2028 -3
============================================
- Hits 9900 9848 -52
- Misses 5295 5296 +1
+ Partials 821 820 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ala/org/apache/kyuubi/events/handler/package.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9wYWNrYWdlLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/spark/kyuubi/SparkContextHelper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrQ29udGV4dEhlbHBlci5zY2FsYQ==) | `52.94% <33.33%> (-4.96%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `65.25% <72.22%> (+0.76%)` | :arrow_up: |
| [.../events/handler/SparkJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0pzb25Mb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `83.33% <83.33%> (ø)` | |
| [...events/handler/ServerJsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9TZXJ2ZXJKc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `83.33% <83.33%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `65.59% <87.50%> (+2.62%)` | :arrow_up: |
| [...main/scala/org/apache/kyuubi/events/EventBus.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvRXZlbnRCdXMuc2NhbGE=) | `88.57% <88.57%> (ø)` | |
| [...yuubi/events/handler/JsonLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9ldmVudHMvaGFuZGxlci9Kc29uTG9nZ2luZ0V2ZW50SGFuZGxlci5zY2FsYQ==) | `82.92% <90.00%> (ø)` | |
| [...ents/handler/SparkHistoryLoggingEventHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvaGFuZGxlci9TcGFya0hpc3RvcnlMb2dnaW5nRXZlbnRIYW5kbGVyLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `84.12% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e8445b7...beabf05](https://codecov.io/gh/apache/incubator-kyuubi/pull/1853?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] wForget commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
wForget commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033521443
> The `EventLogger` s, such as `JsonEventLogger` extends `AbstractService` now which makes it hard to developers to implement. we shall make it AbstractService-free.
>
> And the EventLoggingService is now sync and we shall make it async simply.
>
> WDYT? @wForget @iodone
+1, I prefer this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033433258
> > cc @wForget, can you help check this PR?
>
> Sorry, I'm confused about this, this is overcorrect for my needs. For me, I just need to store the event asynchronously and don't need to add some handlers to handle it when producing events. I will read the event in storage(like es), then process and audit it.
>
> In addition, I feel this is similar to before:
>
> 1. EventBus is similar to EventLoggingService
> 2. EventHandler is similar to EventLogger
@wForget The motivation for the implementation of eventBus can be found in #1798
EventBus is just a Handler for triggering and adding events, it does not contain any business logic, EventLoggerService can be seen as a concrete business implementation of one of these Handlers. Because the previous EventLoggerService implementation was slightly complex and coupled, it was adapted using EventBus in order to be compatible with the previous implementation logic, and at the same time, it is very clear and simple to use EventBus.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1079592789
> LGTM, in addition, I have some comments:
>
> 1. Why don't we register handlers with `EventBus.registerAsync`?
> 2. `SparkJsonLoggingEventHandler` and `ServerJsonLoggingEventHandler` don't seem to do anything, they seem unnecessary.
> 3. Do we need to use a separate `ExecutionContext` for EventBus instead of `Implicits.global`?
@wForget, thank you for your advice.
For points 1, 3:
Currently, since the Async pattern has not been validated in real scenarios:
1. Just the same implementation of the previous LoggerService is done, not upgraded to asynchronous.
2. Scala global `executioncontext` uses the `ForkJoin` settings related to the system, which can ensure optimal performance. We can tuning it when using async EventHandler in the future like the implement of `AsyncElasticSearchLoggingHandler`
For points 2:
For decoupling and extensibility, while also facilitating independent unit testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] wForget commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
wForget commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1079550911
LGTM, in addition, I have some comments:
1. Why don't we register handlers with `EventBus.registerAsync`?
2. `SparkJsonLoggingEventHandler` and `ServerJsonLoggingEventHandler` don't seem to do anything, they seem unnecessary.
3. Do we need to use a separate `ExecutionContext` for EventBus instead of `Implicits.global`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1053996753
Sorry for the late reply.
the idea is great, but can we make it a bit simple. As `EventLogger` can be a typical `EventHandler`, we can remove one layer(EventLoggerService) directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#discussion_r802359490
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/events/EventBus.scala
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.events
+
+import scala.collection.mutable
+import scala.concurrent.{ExecutionContext, Future}
+import scala.reflect.{classTag, ClassTag}
+import scala.util.Try
+
+import org.apache.kyuubi.Logging
+import org.apache.kyuubi.util.ThreadUtils
+
+sealed trait EventBus {
+ def post[T <: KyuubiEvent](event: T): Unit
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus
+}
+
+object EventBus extends Logging {
+ private val defaultEventBus = EventBusLive()
+
+ def apply(): EventBus = EventBusLive()
+
+ // Exposed api
+ def post[T <: KyuubiEvent](event: T): Unit = defaultEventBus.post[T](event)
+
+ def register[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.register[T](et)
+
+ def registerAsync[T <: KyuubiEvent: ClassTag](et: EventHandler[T]): EventBus =
+ defaultEventBus.registerAsync[T](et)
+
+ private case class EventBusLive() extends EventBus {
+ private[this] val eventHandlerRegistry = new Registry
+ private[this] val asyncEventHandlerRegistry = new Registry
+ implicit private[this] val ec: ExecutionContext =
+ ExecutionContext.fromExecutor(
+ ThreadUtils.newDaemonQueuedThreadPool(10, 10, 60000L, "event-bus-exec-pool"))
Review comment:
For asynchronous processing is relatively simple to implement, the size of the thread pool is configured by kyuubi Config or changed to a ForkJoin thread pool?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033703963
The definition of `EventHandler` is a function with few constraints, and the input is `KyuubiEvent`, which is a fully AbstractService-free:
```scala
package object events {
type EventHandler[T <: KyuubiEvent] = T => Unit
}
```
We can implement `EventHandler`, either following the `AbstractService` pattern, or freely writing any implementation pattern.
In this commit it is easy to convert the previously implemented `EventLoggerService` into an `EventHandler` without any intrusiveness.
```scala
private val eventLoggingService =
new EventLoggingService with EventHandler[KyuubiServerEvent] {
override def apply(event: KyuubiServerEvent): Unit = onEvent(event)
}
```
@yaooqinn @wForget Does this explanation for `EventBus` meet your expectations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on pull request #1853: [KYUUBI #1798] Add EventBus module to unify the distribution and subscription of Kyuubi's events
Posted by GitBox <gi...@apache.org>.
iodone commented on pull request #1853:
URL: https://github.com/apache/incubator-kyuubi/pull/1853#issuecomment-1033567479
> The `EventLogger` s, such as `JsonEventLogger` extends `AbstractService` now which makes it hard to developers to implement. we shall make it AbstractService-free.
>
> And the EventLoggingService is now sync and we shall make it async simply.
>
> WDYT? @wForget @iodone
This `AbstractService` is actually the `EventHandler` implemention in `EventBus`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org