Skip to content

[cleanup]Remove Trino/PrestoDB Pulsar plugin from main repo #21795

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.

Sign up for GitHub

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

Jump to bottom
Merged
merlimat merged 1 commit into from
Dec 25, 2023
Merged

[cleanup]Remove Trino/PrestoDB Pulsar plugin from main repo #21795

merlimat merged 1 commit into from
Dec 25, 2023

Conversation

@merlimat
Copy link
Contributor

@merlimat merlimat commented Dec 23, 2023 •
edited
Loading

PIP: 62: https://github.com/apache/pulsar/wiki/PIP-62%3A-Move-connectors%2C-adapters-and-Pulsar-Presto-to-separate-repositories

Motivation

Described again in mailing list discussion https://lists.apache.org/thread/4f1cco12cycq36m7vtyjs2j5q5975666

Modifications

Removed the Trino Pulsar plugin

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@merlimat merlimat added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use release/important-notice The changes which are important should be mentioned in the release note ready-to-test labels Dec 23, 2023
@merlimat merlimat added this to the 3.2.0 milestone Dec 23, 2023
@merlimat merlimat self-assigned this Dec 23, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2023 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.56%. Comparing base (8beac8b) to head (c632659).
⚠️ Report is 1360 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21795      +/-   ##
============================================
+ Coverage     73.42%   73.56%   +0.13%     
+ Complexity    32795    32235     -560     
============================================
  Files          1897     1857      -40     
  Lines        140656   138003    -2653     
  Branches      15491    15109     -382     
============================================
- Hits         103282   101523    -1759     
+ Misses        29297    28617     -680     
+ Partials       8077     7863     -214
Flag Coverage Δ
inttests 24.17% <ø> (-0.08%) ⬇️
systests 23.66% <ø> (-1.18%) ⬇️
unittests 72.85% <ø> (+0.15%) ⬆️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/offload/OffloaderUtils.java 63.23% <ø> (ø)

... and 70 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@crossoverJie
Copy link
Member

crossoverJie commented Dec 23, 2023

We are users of Pulsar-SQL (although I also feel that active users are not many).

And now there are still some issues waiting to be fixed. I hope it can be split into separate repositories so that these issues can continue to be fixed.

@Technoboy-
Copy link
Contributor

Technoboy- commented Dec 25, 2023 •
edited
Loading

Add related security 3.0.2 and 3.1.1 has 3 fixable security vulnerabilities with Trino, it's hard to upgrade from 368 to 430

@merlimat
Copy link
Contributor Author

merlimat commented Dec 25, 2023

@crossoverJie As I mentioned in the mailing list discussion, if there are volunteers that can help maintain the Trino plugin, I can definitely help set the new repo up and import the code.

@merlimat merlimat merged commit c157f0d into apache:master Dec 25, 2023
@compuguy
Copy link

compuguy commented Dec 26, 2023 •
edited
Loading

Add related security 3.0.2 and 3.1.1 has 3 fixable security vulnerabilities with Trino, it's hard to upgrade from 368 to 430

That's what I thought. I honestly had a hard time going through the change logs to attempt to find where the vulnerabilities are fixed. Update on #21457, I believe Trino only adds two vulnerabilities. I think CVE-2023-0833 doesn't apply to Trino and Pulsar respectively.

We are users of Pulsar-SQL (although I also feel that active users are not many).

And now there are still some issues waiting to be fixed. I hope it can be split into separate repositories so that these issues can continue to be fixed.

We had one of our tenants ask about Pulsar SQL functionality, but we came to the conclusion it wasn't worth the effort adding Trino to our current Pulsar deployments.

@lhotari lhotari mentioned this pull request Dec 28, 2023
Merged
4 tasks
@lhotari lhotari mentioned this pull request May 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@tisonkun tisonkun Awaiting requested review from tisonkun

@onobc onobc Awaiting requested review from onobc

Assignees

@merlimat merlimat

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test release/important-notice The changes which are important should be mentioned in the release note type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

6 participants

@merlimat @codecov-commenter @crossoverJie @Technoboy- @compuguy @poorbarcode