-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove Vertica connector #26904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Remove Vertica connector #26904
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR excises the unmaintained Vertica connector by stripping its module and dependency entries, purging related provisioning, documentation, CI/test configurations, and removing the plugin implementation and tests. Class diagram for removed Vertica connector classesclassDiagram
class VerticaPlugin {
+getConnectorFactories()
}
class VerticaClient {
+VerticaClient()
+getConnection()
+executeQuery()
}
class VerticaClientModule {
+configure()
}
class VerticaDriver {
+connect()
+disconnect()
}
class VerticaTableStatisticsReader {
+readStatistics()
}
%% Test classes
class BaseVerticaConnectorSmokeTest {
+testBasicQueries()
}
class TestVerticaConnectorSmokeTest {
+testSmoke()
}
class TestVerticaConnectorTest {
+testConnector()
}
class TestVerticaLatestConnectorSmokeTest {
+testLatestSmoke()
}
class TestVerticaPlugin {
+testPluginLoad()
}
class TestVerticaTableStatistics {
+testStatistics()
}
class TestVerticaTypeMapping {
+testTypeMapping()
}
class TestingVerticaServer {
+start()
+stop()
}
class VerticaQueryRunner {
+runQuery()
}
%% All above classes were removed in this PR
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
FYI, https://trinodb.slack.com/archives/C07ABNN828M/p1755729147128999 is the relevant Slack thread. I'm waiting for a reply from OpenText. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we were to add this connector today, lack of testability options would be a blocker
for that reason, I consider this a rational move.
keeping it inline, but without ability to run tests, will make developing new features and new connector changes harder.
The connector may find new home outside of core trino repo, but still within trinodb org. It should be removed from here first, to define clear cut off version. Additionally, such a move should not be done, if there are no enough volunteers wishing to maintain the connector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was significant effort involved to get this connector into Trino and OpenText was very interested in having the connector in Trino. They changed the license of the JDBC driver to allow this to proceed and had significant plans to enhance the connector and work on it. I think the change to make the container no longer available is probably not understanding the impact. We need to reach out and clarify this with their team and ensure they container becomes available again. If not they need to figure out how to proceed understanding that we can not continue to have the connector in the codebase..
Until we discussed this with them I think we should NOT remove the connector even if we just deactivate the tests for it for now. From what I know @ebyhr reached out. Please include myself and also potentially @martint @dain and @electrum in any communication with them so we can get some reasonable commitment from them to help on the work.
|
They might have had such enhancement plans, but in reality, there was no contribution as you can see here: https://github.com/trinodb/trino/pulls?q=vertica+is%3Apr+is%3Aclosed I sent an email to OpenText on Oct 10 (no response) and a reminder on Oct 21 (no response). |
|
I posted on Linkedin for the TSF and myself to see if we can get help from OpenText. Personally I would leave would vote for deactivating the test and leaving the connector in for a bit longer - at least until we hear back from them one way or another. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We heard back from OpenText now and are working towards a solution. Until we know if that solution will work or not, we should NOT merge this PR and the removal. Worst case for now we ship the connector without test coverage.
Great, what's the timeline?
That would be a bad precedence. We do not accept connectors that we cannot test, because we do not ship stuff that might not work. |
We are waiting for OpenText to get back to us.
Sure.. but the connector is already here and we do not just throw them out without careful consideration either. There was significant work involved getting the connector in and removing it seems like a waste of time, especially since the vendor wants to collaborate. Maybe we have to find a means for community connectors outside the core, or even for now just deactivate the tests and exclude the connector from the shipped binaries. In either case .. I am against a hurried removal just so some vendor can then support it again on their own. Trino should be a community effort. |
e297c80 to
f8c6438
Compare
Description
I propose removing the Vertica connector since OpenText has removed all Docker images from Docker Hub.
We shouldn't release an untested connector.
vertica/vertica-containers#64 (comment) is a comment from OpenText:
Release notes
Summary by Sourcery
Remove the unsupported Vertica connector and all related build modules, code, tests, CI configuration, and documentation.
Build:
CI:
Documentation:
Tests: