HostChooser as a plugin enhancement in pgjdbc driver by kneeraj · Pull Request #1 · kneeraj/pgjdbc

kneeraj · 2024-09-16T13:19:52Z

All Submissions:

- Have you followed the guidelines in our Contributing document?
- Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

- Does your submission pass tests?
- Does ./gradlew styleCheck pass ?
- Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

- Does this break existing behaviour? If so please explain.
- Have you added an explanation of what your changes do and why you'd like us to include them?
  Github Issue with details.
- Have you written new tests for your core changes, as applicable?
- Have you successfully run tests with your changes locally?

properties

…operty.

iSignal · 2024-10-08T01:54:06Z

+   *
+   * @return true or false.
+   */
+  boolean isHostDrainingConnections(String host);


minor: maybe this can just be called isValid(String host) to make it easy to understand what it maps to.

I was also thinking the same. Let me change it.

iSignal · 2024-10-08T01:57:40Z

+            (HostChooser) ((QueryExecutorImpl) this.queryExecutor).getHostChooser();
+        if (hc != null) {
+          String host = queryExecutor.getHostSpec().getHost();
+          return !hc.isHostDrainingConnections(host);


Given some conn pools like Hikari don't call isValid because this implementation of executing an empty prepared statement can involve a server round trip (as I understand it), should we allow the hostchooser isValid to also control whether this prepared statement execution is done in line 1553 above. For ex, if the hostchooser can return an is_valid, is_valid_recheck, is_invalid response and only if the response is is_valid_recheck, the empty prepared statement is executed. This can allow a user to set the Hikari setting com.zaxxer.hikari.aliveBypassWindowMs to 0 with no perf penalty and the host chooser can use its own information to decide if a host is still valid, perhaps by running periodic checks in the background.

Nice point. Let me try to add this approach.

iSignal · 2024-10-08T02:05:06Z

+public class CustomHostChooserManager {
+  private final Map<HostChooserUrlProperty, HostChooser> hostChooserMap =
+      new WeakHashMap<>();
+  private static CustomHostChooserManager instance = getInstance();


does instance need to be initialized up front? Can it be initialized in a lazy way?

Also does instance need to be marked volatile?

If we change it to
private static final
then volatile is not required. I think it is not required even otherwise as this is the first time initialization and it is a singleton instance.

On demand creation can be done but this is just one manager object. It should be ok I think.

iSignal · 2024-10-08T02:14:00Z

+
+  public HostChooser getHostChooser(HostChooserUrlProperty customImplClassName) {
+    if (hostChooserMap.containsKey(customImplClassName)) {
+      return hostChooserMap.get(customImplClassName);


Is it ok to use this hash map without synchronization? Given that a different thread may be currently modifying this, the result of this lookup may be invalid or inaccurate.

It is called in the getOrCreateHostChooser method. There we are synchronizing it in case it is null. We can avoid costly synchronization when it is not required most of the times.

Can there be two host choosers in the same process? It seems so because the property can be changed on each conn?

If one thread is modifying the map to insert a new host chooser and another is trying to read the map, this part of the code which operates without synchronization can see unexpected results.

If you want to keep an optimized path for the 99% case where there is just one host chooser, I would not use a map and instead do something like

cachedHostChooser = { className, obj } ; concurrenthashmap<....>

where the first plugin is written to the cachedHostChooser and the get path looks up the cachedHostChooser without synchronization. If it is not found there, it can lookup the concurrent hash map in a safe way.

A few reasons for why multiple HostChoosers has value

It’s not uncommon to connect to ybdb and a postgresql instance both from the same app. We got this explicit ask from a customer recently albeit for ruby driver.

Pgjdbc maintainers initially didn't like us using the postgresql url protocol jdbc:postgresql prefixed url for yugabytedb. They said that we are forcing the user to use our flavour of the driver to connect to a postgresql db also in case the app wants to connect to both postgresql and yugabytedb, which made a lot of sense.

I personally liked this flexibility because a yb user can use some implementation of our HostChooser for one universe or may want to modify it a bit for a different universe.

If the user has primary secondary pg setup itself for high availability then they may want to handle unavailability of servers as per their own logic and so on…

Also it doesn't look like that the sync is necessary. A 'null' value when lookup is done will enforce the creation inside a synch block. And a value which is modified inside the synch block is written back to the main memory and it invalidates all the cache values. So to me it looks like we can avoid the synchronization and the current code looks ok.

…pgjdbcgh-3367

ashetkar · 2024-10-07T03:42:40Z

        GlobalHostStatusTracker.reportHostStatus(hostSpec, hostStatus);
        knownStates.put(hostSpec, hostStatus);
        if (!candidateHost.targetServerType.allowConnectingTo(hostStatus)) {
+          // Log target server type did not match


Is this a todo or the comment suggests to log a message from inside registerFailure()?

Shall remove it. Got left behind accidentally. Thanks.

ashetkar · 2024-10-07T03:43:59Z

+   */
+  boolean isHostDrainingConnections(String host);
+
+  boolean isInbuilt();


Javadoc for this? Why is it needed?

I just thought having it for future use would be good where in certain things the driver might not want to do if the plugin is inbuilt. BTW currently also it is being used here:
if (!hostChooser.isInbuilt()) { key.setHostChooser(hostChooser); }

…:classes'

kneeraj and others added 11 commits September 16, 2024 18:47

HostChooser plugin first cut

d251fdc

Some refactoring to make the weakhashmap use a key made of url and

57a3147

properties

Corrected the cll of registerFailures at a couple of places.

4ef4da7

Fixed getProps() and getHostChooser()

61894e0

PGClusterAwareDataSource class implementation

decfdff

Unit test added.

af70c54

Added java doc and updated unit test.

8312361

Style check changes in unit test.

1142a01

Fixed styleCheck issues and minor api corrections.

d44d0f4

Fixed some styleCheck issues.

7700230

Added null check in PgConnection.close()

160b964

kneeraj changed the title ~~HostChooser plugin first cut~~ HostChooser as a plugin enhancement in pgjdbc driver Oct 3, 2024

HarshDaryani896 added 2 commits October 3, 2024 13:03

Added null check in PGConnection.isValid() and moved HostChooser PGPr…

e8dc2be

…operty.

Use PGClusterAwareDataSource class in dataSourceProperties() test.

f2becde

iSignal reviewed Oct 8, 2024

View reviewed changes

kneeraj added 2 commits October 8, 2024 12:15

Took care of review comments from Sanketh I.

c72707a

Merge branch 'pgjdbcgh-3367' of https://github.com/kneeraj/pgjdbc into …

e7e840b

…pgjdbcgh-3367

HarshDaryani896 reviewed Oct 8, 2024

View reviewed changes

Comment thread pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java Outdated

Added null check in PgConnection.IsValid() and styleCheck changes.

fe8a812

ashetkar reviewed Oct 10, 2024

View reviewed changes

kneeraj and others added 5 commits October 15, 2024 13:56

minor cleanups

efbaffb

Hostchooser test class name change

f4ce6cc

Fixed errors shown by './gradlew -PenableCheckerframework :postgresql…

d2ce799

…:classes'

Merge branch 'master' into pgjdbcgh-3367

2d319a7

StyleCheck monir changes

d67a396

Conversation

kneeraj commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

New Feature Submissions:

Changes to Existing Features:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kneeraj commented Sep 16, 2024 •

edited

Loading