yugabyte / jdbc-yugabytedb

JDBC Driver for Yugabyte SQL (YSQL)
BSD 2-Clause "Simplified" License
17 stars 7 forks source link

Do not publish to the org.postgresql package namespace #14

Open lukaseder opened 3 years ago

lukaseder commented 3 years ago

I'm submitting a ...

Describe the issue

This driver is currently copying the official PostgreSQL JDBC driver and publishing it under the same package namespace under org.postgresql: https://github.com/yugabyte/jdbc-yugabytedb/tree/master/jdbc-yugabytedb/src/main/java/org/postgresql

This has always been discouraged in the Java ecosystem for various reasons:

When someone needs to include the official driver and yours in the same classpath, or worse, the same module path, then there will be namespacing conflicts between the two drivers. At the latest when pgjdbc decides to publish itself as a Java 9 module, then you won't be able to continue this practice.

Driver Version?

42.2.7-yb-6-SNAPSHOT

Java Version?

N/A

OS Version?

N/A

PostgreSQL Version?

N/A

To Reproduce

See: https://github.com/yugabyte/jdbc-yugabytedb/tree/master/jdbc-yugabytedb/src/main/java/org/postgresql

Expected behaviour

You have two acceptable options:

The latter is called "shading" and there's a handy Maven plugin for that: https://maven.apache.org/plugins/maven-shade-plugin

Logs

N/A

davecramer commented 3 years ago

+1 to not using the org.postgresql namespace. Shading your driver would be fine. I think the pgjdbc project would be open to adding hooks to enable something like this. It seems that the need is real as this project is not the first to do it.

kneeraj commented 3 years ago

Hi @lukaseder and others,

Sorry for a delayed response here. Was caught up in few things.

Coming to the issue:
As you all know we just published the beta version of this driver sometime ago. It is a work in progress and any comments on that is very welcome at this stage, so first of all thanks for your inputs.

Making the changes on the fork of the official driver is definitely the way forward and was already in our plan. We were just trying to conduct some experiments on an existing copy of the source code, which was there for some historical reasons, but rest assured the next release will be from a fork and not a copy.

However we would really like to explore this ( below ) suggestion further:

Depend on the official driver and extend it via its extension points (less powerful, of course, if you need to patch stuff)

Just to be clear, I believe you are talking about new extension points (java SPI?) which can be added in the upstream jdbc driver for such purposes. Right? We would like to understand the proposal better and will be happy to collaborate also on this.

'shading' may not be a preferred option for us as it might break compatibilities with other database tools.

Also, our plan is to extend the driver for a couple of more things specific to YugabyteDB, in future. And even they seem like generic requirements.

It will be great to talk about these as well along with the in-built 'load-balancing' feature.

BTW the current implementation of load-balance is a very simple approach we have taken and it is almost like an extension approach. It will be great if you can find out sometime and we can discuss this.

Looking forward to your response on this.

davecramer commented 3 years ago

I have to say that using the same URL and namespace as the pgjdbc driver is not making me happy. As @lukaseder pointed out there are legal and trademark issues here. Not to mention the mess this would make in an application.

As for extending the driver. We have discussed it, and would entertain proposals.

lukaseder commented 3 years ago

Just to be clear, I believe you are talking about new extension points (java SPI?) which can be added in the upstream jdbc driver for such purposes. Right?

Yes that's what I had in mind

'shading' may not be a preferred option for us as it might break compatibilities with other database tools.

I take such "breakages" any time over split packages in Maven Central, if you don't own the namespace:

  1. Few people depend on pgjdbc API directly, most use only JDBC API
  2. If pgjdbc API is needed, then it's easy to search/replace the package name in a project from org.postgresql to com.yugabyte or whatever
  3. While package renames can be fixed easily in client code, split packages cannot, once you're on the module path. There really isn't a good solution to the problem you've created for users who have both drivers on the classpath/module path.
  • Data aware query routing - The driver detects which server is the best server for any read/write op to go to rather than the default one.

JDBC 4.3 has sharding support, maybe you mean this? In any case, that's probably interesting to implement in a data source / connection pool, rather than the JDBC driver directly. This can definitely be implemented in any namespace, so I don't think an SPI is required.

  • Automatic failover to a healthy server in case connectivity to one is lost for any reason.

Sounds again like a job for a data source / connection pool, rather than the JDBC driver directly.

davecramer commented 3 years ago

Honestly I believe all of these things do not belong in the driver. There are a number of pools that implement read/write splitting of statements. There are a number of tools that implement failover. Using the driver seems to be the wrong tool IMO

kneeraj commented 3 years ago

Ok. We will be having the driver packaged under yugabyte namespace and we will use a different url. Thanks.

davecramer commented 2 years ago

Ok. We will be having the driver packaged under yugabyte namespace and we will use a different url. Thanks.

Apparently the driver is being distributed using the same URL and namespace still.

kneeraj commented 2 years ago

No it is not. This repo clearly mentions in the README:

YugabyteDB JDBC Driver [Deprecated] - Please consider using the new YugabyteDB JDBC Driver from https://github.com/yugabyte/pgjdbc

We are releasing our new driver from this fork https://github.com/yugabyte/pgjdbc Please have a look at the docs and the README there. Both the url and driver class has changed. The url starts from jdbc:yugabytedb and Driver class is com.yugabyte.Driver We will be completely removing it very soon.

davecramer commented 2 years ago

Excellent, thanks!