Skip to content
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

Added support for JDBC, added sample jar for Hazelcast #6854

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

devops-42
Copy link

@devops-42 devops-42 commented Apr 6, 2024

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

This PR brings a new Data source JDBC into redash. This allows to connect to sources offering a JDBC interface.

How is this tested?

Testing is coming soon.

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 48.48485% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 63.78%. Comparing base (15e6583) to head (a6104a5).
Report is 1 commits behind head on master.

❗ Current head a6104a5 differs from pull request most recent head 0f5e04d. Consider uploading reports for the commit 0f5e04d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6854      +/-   ##
==========================================
- Coverage   63.82%   63.78%   -0.04%     
==========================================
  Files         161      162       +1     
  Lines       13060    13093      +33     
  Branches     1803     1806       +3     
==========================================
+ Hits         8335     8351      +16     
- Misses       4425     4442      +17     
  Partials      300      300              
Files Coverage Δ
redash/serializers/__init__.py 78.51% <ø> (ø)
redash/settings/__init__.py 98.64% <ø> (ø)
redash/query_runner/jdbc.py 48.48% <48.48%> (ø)

@justinclift
Copy link
Member

@devops-42 Good to see enthusiasm. 😄

While you're at it, please fill out the fields in the top comment so the other developers have some idea what this is for. 😁


Would you be ok to add further tests to this, to increase the code coverage and get the codecov checks to pass?

Dockerfile Outdated Show resolved Hide resolved
@justinclift
Copy link
Member

justinclift commented Apr 7, 2024

Interestingly, it looks like Hazelcast have moved to be Open Core:

https://hazelcast.com/blog/changes-to-community-edition/

The "not making CVE fixes available to the Community until the next point release" sounds super dangerous. 😦

At first read, the other items sound reasonable (to me) though.

@justinclift
Copy link
Member

justinclift commented Apr 7, 2024

On a different note, it looks like there are official Hazelcast docker images:

https://hub.docker.com/r/hazelcast/hazelcast/

We could potentially use those for automatically testing this new data source, to make sure it doesn't bit rot over time to the point of breakage.


Looks like there's an official Hazelcast Python (client) library too:

https://pypi.org/project/hazelcast-python-client/

@devops-42 Would it have been better to use that, rather than go the JDBC route?

Am guessing you've seen that already though, and figured JDBC was the better approach here. 😄

Dockerfile Outdated Show resolved Hide resolved
@devops-42
Copy link
Author

On a different note, it looks like there are official Hazelcast docker images:

https://hub.docker.com/r/hazelcast/hazelcast/

We could potentially use those for automatically testing this new data source, to make sure it doesn't bit rot over time to the point of breakage.

Looks like there's an official Hazelcast Python (client) library too:

https://pypi.org/project/hazelcast-python-client/

@devops-42 Would it have been better to use that, rather than go the JDBC route?

Am guessing you've seen that already though, and figured JDBC was the better approach here. 😄

Hey @justinclift,

have seen it. I considered to go the JDBC way to have a data source, which might be used for other JDBC connections too. :)

@devops-42 devops-42 closed this Apr 7, 2024
@devops-42 devops-42 reopened this Apr 7, 2024
@devops-42
Copy link
Author

Interestingly, it looks like Hazelcast have moved to be Open Core:

https://hazelcast.com/blog/changes-to-community-edition/

The "not making CVE fixes available to the Community until the next point release" sounds super dangerous. 😦

At first read, the other items sound reasonable (to me) though.

The future handling of CVE is indeed an issue 😐 Maybe the community is strong enough to mitigate this.

@justinclift
Copy link
Member

I considered to go the JDBC way to have a data source, which might be used for other JDBC connections too. :)

No worries, as long as it works reliably. 😄

@arikfr
Copy link
Member

arikfr commented May 17, 2024

I appreciate the enthusiasm here and the effort put already, but I don't think we should merge this. The usefulness of a JDBC connector is very limited -- I doubt there are meaningful databases that have only JDBC connector and don't have alternative options. But on the other hand it will bloat our Docker image and add extra dependencies to maintain over time, that I doubt many on the current team use or familiar with.

An alternative path for you (or whoever interested in JDBC) is to build a custom image on top of the official one that adds the JDBC dependencies and the JDBC query runner. The way Redash is built it should be possible to do so without having to build your own image from scratch.

@justinclift
Copy link
Member

justinclift commented May 17, 2024

It's been a few months since the Hazelcast people announced they're not including security fixes (not even urgent ones) in their Community release. Looking at their Feature Comparison page, it seems like they're not changing that stance to anything reasonable:

https://hazelcast.com/products/feature-comparison/

Personally, I don't reckon we should support an organisation that deliberately puts their users at risk. As in, lets not merge any form of this, whether it be JDBC based, Python based, etc. 😄

Or am I being unreasonable? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants