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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
