UOETianleZhang commented on code in PR #15960:
URL: https://github.com/apache/pinot/pull/15960#discussion_r2138467702
##########
pinot-integration-tests/pom.xml:
##########
@@ -177,6 +177,14 @@
<groupId>org.apache.pinot</groupId>
<artifactId>pinot-segment-writer-file-based</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-timeseries-m3ql</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.pinot</groupId>
+ <artifactId>pinot-timeseries-planner</artifactId>
+ </dependency>
Review Comment:
Hi @shauryachats, I noticed that **pinot-timeseries-m3ql** (a plugin module)
was added as a dependency to **pinot-integration-tests** (our core test suite
so it is a core module).
Since plugins are optional by design, introducing them into the core modules
comes with a few concerns:
- Plugin modules would now introduce transitive dependencies to core modules
and brings unecessary dependencies. It is hard for the the consumer of OSS
Pinot to exclude those dependcies
- Tightly coupling plugin tests with the core suite undermines our ability
to evolve or replace plugins independently.
Instead of adding new plugin deps to the core suite, could we:
1. Add a dedicated submodule under pinot-integration-tests for
non-core plugins (e.g., pinot-integration-tests/plugins), or
2. Add a sperate test package for non-core modules
and move the pinot-timeseries-m3ql tests there? In this way plugin tests
will have seperate pom.xml and consumers can easily exclude them.
I know pinot-integration-tests has already included some plugin
dependencies. We should clean them up seperately but let’s avoid adding new
ones for now.
With the above we should be able to keep our core tests lean and
maintainable :) Let me know how you think!
cc @ankitsultana
--
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]
