search

apache / pinot

Apache Pinot - A realtime distributed OLAP datastore
https://pinot.apache.org/
Apache License 2.0
5.2k stars 1.21k forks source link

Remove tyrus dependencies in pinot-tools module #6139

Closed jackjlli closed 3 years ago

jackjlli commented 3 years ago

Description

This PR is to remove the tyrus dependencies, because it contains a class that happens to have the same class name ServerConfiguration and same method as the one of org.glassfish.jersey.containers:jersey-container-grizzly2-http.

That's why the following exception is thrown when running RealtimeQuickStart.


Exception in thread "main" java.lang.NoSuchMethodError: org.glassfish.grizzly.http.server.ServerConfiguration.addHttpHandler(Lorg/glassfish/grizzly/http/server/HttpHandler;[Lorg/glassfish/grizzly/http/server/HttpHandlerRegistration;)V
    at org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory.createHttpServer(GrizzlyHttpServerFactory.java:258)
    at org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory.createHttpServer(GrizzlyHttpServerFactory.java:114)
    at org.apache.pinot.controller.api.ControllerAdminApiApplication.start(ControllerAdminApiApplication.java:115)
    at org.apache.pinot.controller.ControllerStarter.setUpPinotController(ControllerStarter.java:416)
    at org.apache.pinot.controller.ControllerStarter.start(ControllerStarter.java:287)
    at org.apache.pinot.tools.service.PinotServiceManager.startController(PinotServiceManager.java:113)
    at org.apache.pinot.tools.service.PinotServiceManager.startRole(PinotServiceManager.java:90)
    at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.lambda$startBootstrapServices$0(StartServiceManagerCommand.java:234)
    at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.startPinotService(StartServiceManagerCommand.java:286)
    at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.startBootstrapServices(StartServiceManagerCommand.java:233)
    at org.apache.pinot.tools.admin.command.StartServiceManagerCommand.execute(StartServiceManagerCommand.java:183)
    at org.apache.pinot.tools.admin.command.StartControllerCommand.execute(StartControllerCommand.java:130)
    at org.apache.pinot.tools.admin.command.QuickstartRunner.startControllers(QuickstartRunner.java:105)
    at org.apache.pinot.tools.admin.command.QuickstartRunner.startAll(QuickstartRunner.java:140)
    at org.apache.pinot.tools.RealtimeQuickStart.execute(RealtimeQuickStart.java:90)
    at org.apache.pinot.tools.RealtimeQuickStart.main(RealtimeQuickStart.java:46)
codecov-io commented 3 years ago

Codecov Report

Merging #6139 into master will increase coverage by 6.30%. The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6139      +/-   ##
==========================================
+ Coverage   66.44%   72.75%   +6.30%     
==========================================
  Files        1075     1229     +154     
  Lines       54773    58025    +3252     
  Branches     8168     8561     +393     
==========================================
+ Hits        36396    42217    +5821     
+ Misses      15700    13022    -2678     
- Partials     2677     2786     +109
Flag Coverage Δ
#integration 45.22% <49.14%> (?)
#unittests 63.93% <38.09%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <ø> (ø)
.../helix/BrokerUserDefinedMessageHandlerFactory.java 52.83% <0.00%> (-13.84%) :arrow_down:
...ava/org/apache/pinot/client/AbstractResultSet.java 53.33% <0.00%> (-3.81%) :arrow_down:
.../main/java/org/apache/pinot/client/Connection.java 44.44% <0.00%> (-4.40%) :arrow_down:
.../org/apache/pinot/client/ResultTableResultSet.java 24.00% <0.00%> (-10.29%) :arrow_down:
...not/common/assignment/InstancePartitionsUtils.java 78.57% <ø> (+5.40%) :arrow_up:
.../apache/pinot/common/exception/QueryException.java 90.27% <ø> (+5.55%) :arrow_up:
...pinot/common/function/AggregationFunctionType.java 98.27% <ø> (-1.73%) :arrow_down:
.../pinot/common/function/DateTimePatternHandler.java 83.33% <ø> (ø)
...ot/common/function/FunctionDefinitionRegistry.java 88.88% <ø> (+44.44%) :arrow_up:
... and 978 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a910f5d...b432f15. Read the comment docs.

kishoreg commented 3 years ago

can you please make sure quickstart (both offline and realtime) works

jackjlli commented 3 years ago

It is unclear on why removing tyrus requires changing MeetupStream to be a static file. Could you explain?

Currently Tyrus is only used for streaming data in RealtimeQuickStart class. It depends on an external source for now (i.e. meetup.com). If anything breaks in the upstream, our build/test will inevitably fail (this is what we faced recently). Plus, the current logic doesn't validate the actual result from this event because the result isn't deterministic; it just evaluates whether realtime table has consumed some data. Emitting some sample events from a sample file should suffice this requirement.

jackjlli commented 3 years ago

can you please make sure quickstart (both offline and realtime) works

I've tested it in both pinot-tools and pinot-distributions modules and all quick start tests work.

mayankshriv commented 3 years ago

can you please make sure quickstart (both offline and realtime) works

I've tested it in both pinot-tools and pinot-distributions modules and all quick start tests work.

Let's also make sure that PerfBenchmarkRunner also works as well.

jackjlli commented 3 years ago

can you please make sure quickstart (both offline and realtime) works

I've tested it in both pinot-tools and pinot-distributions modules and all quick start tests work.

Let's also make sure that PerfBenchmarkRunner also works as well.

That's been tested as well.

Jackie-Jiang commented 3 years ago

I'm against changing the data to a static file. The purpose of the quick start is letting the user play with Pinot and they should be able to see the latest events from a real stream instead of the unchanged records. Is there a way to solve this issue without hardcode the data file? Also, I'm not able to reproduce this issue locally. Did I miss something?

jackjlli commented 3 years ago

@Jackie-Jiang The root cause is that in some machine the tyrus jar appears before jersey-container-grizzly2-http. The reason why this issue cannot be reproduced in every machine is that each OS controls the order of jars listed in the classpath.

mayankshriv commented 3 years ago

I'm against changing the data to a static file. The purpose of the quick start is letting the user play with Pinot and they should be able to see the latest events from a real stream instead of the unchanged records. Is there a way to solve this issue without hardcode the data file? Also, I'm not able to reproduce this issue locally. Did I miss something?

@Jackie-Jiang The tyrus dependency causes a lot of issues for me as well, e.g. when running perfBenchmarkRunner locally. I agree we should have recipe/setup/demo using live stream. However, note that quickStart is also being used as a test, which makes it brittle if upstream has an issue (we had this recently).

My recommendation is to have the test part of quick-start read data from a deterministic source (e.g. local stream or static file). However, in this case, quick-start (as a user demo tool) remains untested. For this, we could have a quick-start test based on real stream only when cutting the release (instead of all commits). What do you think?

Jackie-Jiang commented 3 years ago

@mayankshriv @jackjlli I'm okay using static file for testing purpose only, but if we remove tyrus, does that mean we can no longer consume real streaming events in the quick-start? I feel that is very bad for demo purpose, or for new users to try out Pinot.

Jackie-Jiang commented 3 years ago

Isn't this just a version conflict? Does excluding this conflicting library from one of the library work?

jackjlli commented 3 years ago

Isn't this just a version conflict? Does excluding this conflicting library from one of the library work?

We cannot exclude either one of them because tyrus is needed to stream external data and jersey-container-grizzly2-http is needed to start the web server.

Jackie-Jiang commented 3 years ago

We cannot exclude either one of them because tyrus is needed to stream external data and jersey-container-grizzly2-http is needed to start the web server.

You can exclude the underlying dependencies of them to resolve the version conflict right?

jackjlli commented 3 years ago

We cannot exclude either one of them because tyrus is needed to stream external data and jersey-container-grizzly2-http is needed to start the web server.

You can exclude the underlying dependencies of them to resolve the version conflict right?

No, both dependencies are required. We cannot exclude either one of them.

Jackie-Jiang commented 3 years ago

@jackjlli I see the problem, basically tyrus copied the code from grizzly and put it in one package. If you upgrade tyrus to 1.15 to be compatible with grizzly 2.4.4, then the problem should be solved

jackjlli commented 3 years ago

@Jackie-Jiang Thanks for looking into it. I've opened a new PR to bump up the Tyrus version to 1.15 (https://github.com/apache/incubator-pinot/pull/6162). Once that's merged, we can close this PR.

jackjlli commented 3 years ago

Closing this PR since https://github.com/apache/incubator-pinot/pull/6162 has been merged.