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

Document migration plan for DataSource.getConnection(String, String) #231

Closed
cowwoc opened this Issue Jan 19, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@cowwoc

cowwoc commented Jan 19, 2015

0fe2765 deprecates getConnection(String, String). Please add Javadoc (@deprecate tag) that explains what users are supposed to do instead.

In the commit message you talk about creating separate pools. I believe it would help users if you mention that they must use HikariConfig.setUsername() and setPassword() to achieve the same result.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 19, 2015

Actually, I think deprecating this method is problematic. If someone wants to implement a wrapper DataSource on top of an existing HikariDataSource instance then you're forcing them to invoke a deprecated method. There is no way to implement a wrapper without doing so, as far as I can see.

For example, in my case I have a wrapper which configures (invokes some SQL code) immediately before returning a new Connection from getConnection(). I'd like this wrapper to know as little as possible about the underlying DataSource.

cowwoc commented Jan 19, 2015

Actually, I think deprecating this method is problematic. If someone wants to implement a wrapper DataSource on top of an existing HikariDataSource instance then you're forcing them to invoke a deprecated method. There is no way to implement a wrapper without doing so, as far as I can see.

For example, in my case I have a wrapper which configures (invokes some SQL code) immediately before returning a new Connection from getConnection(). I'd like this wrapper to know as little as possible about the underlying DataSource.

@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge

brettwooldridge Jan 20, 2015

Owner

@cowwoc Basically, the current implementation of HikariCP's getConnection(String, String) creates a unique pool instance for every unique username/password tuple. A wrapper can do exactly this same thing, using the underlying getConnection() form.

A sample fragment of such an implementation would be:

public class MultiDataSource implements DataSource {
   private HashMap<String, HikariDataSource> mapping; // a ConcurrentHashMap etc. would be better

   // Not thread-safe, but you can get the idea...
   public Connection getConnection(String username, String password) {
      final String poolKey = username + ':' + password;
      HikariDataSource ds = mapping.get(key);
      if (ds == null) {
         ds = new HikariDataSource();
         ds.setUsername(username);
         ds.setPassword(password);
         mapping.put(key, ds);
      }

      return ds.getConnection();
   }

   // ...
}

This is basically what HikariCP does now, but we'd like to get rid of the code. The existing implementation in HikariCP is inadequate in a number of ways, and addressing them all adds more complexity to the code than the value it delivers.

The fact that it works now is only because virtually nobody uses it, and those that do haven't poked it in just the right way to provoke errors. Try configuring pool MBean registration, or Codehale Metrics, and then using the getConnection(username, password) form -- something somewhere will blow-up. These are use-cases that can be covered easily by a user-supplied wrapper in a better way than any solution we could provide.

Owner

brettwooldridge commented Jan 20, 2015

@cowwoc Basically, the current implementation of HikariCP's getConnection(String, String) creates a unique pool instance for every unique username/password tuple. A wrapper can do exactly this same thing, using the underlying getConnection() form.

A sample fragment of such an implementation would be:

public class MultiDataSource implements DataSource {
   private HashMap<String, HikariDataSource> mapping; // a ConcurrentHashMap etc. would be better

   // Not thread-safe, but you can get the idea...
   public Connection getConnection(String username, String password) {
      final String poolKey = username + ':' + password;
      HikariDataSource ds = mapping.get(key);
      if (ds == null) {
         ds = new HikariDataSource();
         ds.setUsername(username);
         ds.setPassword(password);
         mapping.put(key, ds);
      }

      return ds.getConnection();
   }

   // ...
}

This is basically what HikariCP does now, but we'd like to get rid of the code. The existing implementation in HikariCP is inadequate in a number of ways, and addressing them all adds more complexity to the code than the value it delivers.

The fact that it works now is only because virtually nobody uses it, and those that do haven't poked it in just the right way to provoke errors. Try configuring pool MBean registration, or Codehale Metrics, and then using the getConnection(username, password) form -- something somewhere will blow-up. These are use-cases that can be covered easily by a user-supplied wrapper in a better way than any solution we could provide.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 20, 2015

@brettwooldridge This is still questionable because:

  1. You're deprecating a standard interface method. Meaning it won't be safe to abstract-away HikariDataSource to a DataSource. That by itself sets off major warning bells for me.
  2. Why is a user constructing a new instance any safer than doing so internally? Aren't you in a better position to know how to do this right?

If you still believe that this approach is best, then you're going to need to add the ability to copy the configuration of one HikariDataSource into another. An implementation of getConnection(username, password) needs to inherit the configuration settings of the original DataSource first, then override the username/password, then return a new connection. We're missing that first piece: the ability to copy/inherit the settings from one HikariDataSource to another. (I prefer a copy constructor, or factory instead of a method that copies the configuration post-construction).

cowwoc commented Jan 20, 2015

@brettwooldridge This is still questionable because:

  1. You're deprecating a standard interface method. Meaning it won't be safe to abstract-away HikariDataSource to a DataSource. That by itself sets off major warning bells for me.
  2. Why is a user constructing a new instance any safer than doing so internally? Aren't you in a better position to know how to do this right?

If you still believe that this approach is best, then you're going to need to add the ability to copy the configuration of one HikariDataSource into another. An implementation of getConnection(username, password) needs to inherit the configuration settings of the original DataSource first, then override the username/password, then return a new connection. We're missing that first piece: the ability to copy/inherit the settings from one HikariDataSource to another. (I prefer a copy constructor, or factory instead of a method that copies the configuration post-construction).

@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge

brettwooldridge Jan 20, 2015

Owner

HikariCP is certainly not alone in not supporting this method. Vibur, for example, does this:

public Connection getConnection(String username, String password) throws SQLException {
   logger.warn("Different usernames/passwords are not supported yet. Will use the configured defaults.");
   return getConnection();
}

And the new Apache DBCP2:

@Override
public Connection getConnection(String uname, String passwd) throws SQLException {
   throw new UnsupportedOperationException();
}

I certainly have no issue providing copy functionality, there is currently a package-scoped copyState() method on HikariConfig (and inherited by HikariDataSource) that can be made public-scoped.

I consider it an open question whether HikariCP can "do it right" in a way that satisfies all users. Let's say the configuration calls for registering MBeans. Currently, HikariCP will use the poolName in the registration. This cannot work if there are actually multiple pools under the covers, so HikariCP would need to do something like poolName + username in the registration. However, because it is actually the username/password tuple that is the unique key for the pool, then it gets more complicated. For example, ds.getConnection("brett", "foo") and ds.getConnection("brett", "bar") actually create two separate pools, so a MBean registration of poolName + username would fail for the second pool due to lack of uniqueness. So, then maybe an integer could be appended, etc. But is this what all users want? We have the same issues for Codahale Metrics registrations.

HikariDataSource exposes evictConnection(Connection), shutdown(), suspendPool() and resumePool(). If the user calls evictConnection() or shutdown(), from what pool should the eviction occur, or which pool should be shutdown? All? Would we be asked to then further add methods to evictConnection(connection, username, password) or shutdown(username, password)?

I think the answers to these questions is likely to vary quite a bit between users. And it is for that reason primarily that I believe it is best left to the user to create a wrapper that controls the behavior in the way they need. Even if HikariCP picked "one true way" to implement these, it increases the complexity of the pool substantially, affecting both testability and predictability.

Owner

brettwooldridge commented Jan 20, 2015

HikariCP is certainly not alone in not supporting this method. Vibur, for example, does this:

public Connection getConnection(String username, String password) throws SQLException {
   logger.warn("Different usernames/passwords are not supported yet. Will use the configured defaults.");
   return getConnection();
}

And the new Apache DBCP2:

@Override
public Connection getConnection(String uname, String passwd) throws SQLException {
   throw new UnsupportedOperationException();
}

I certainly have no issue providing copy functionality, there is currently a package-scoped copyState() method on HikariConfig (and inherited by HikariDataSource) that can be made public-scoped.

I consider it an open question whether HikariCP can "do it right" in a way that satisfies all users. Let's say the configuration calls for registering MBeans. Currently, HikariCP will use the poolName in the registration. This cannot work if there are actually multiple pools under the covers, so HikariCP would need to do something like poolName + username in the registration. However, because it is actually the username/password tuple that is the unique key for the pool, then it gets more complicated. For example, ds.getConnection("brett", "foo") and ds.getConnection("brett", "bar") actually create two separate pools, so a MBean registration of poolName + username would fail for the second pool due to lack of uniqueness. So, then maybe an integer could be appended, etc. But is this what all users want? We have the same issues for Codahale Metrics registrations.

HikariDataSource exposes evictConnection(Connection), shutdown(), suspendPool() and resumePool(). If the user calls evictConnection() or shutdown(), from what pool should the eviction occur, or which pool should be shutdown? All? Would we be asked to then further add methods to evictConnection(connection, username, password) or shutdown(username, password)?

I think the answers to these questions is likely to vary quite a bit between users. And it is for that reason primarily that I believe it is best left to the user to create a wrapper that controls the behavior in the way they need. Even if HikariCP picked "one true way" to implement these, it increases the complexity of the pool substantially, affecting both testability and predictability.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 20, 2015

@brettwooldridge My understanding of getConnection(username, password) is that the returned connection is meant to belong to the same pool (as opposed to spawning a separate pool as you are expecting).

Right now when a user requests a connection, you grab one from a list of idle connections. In order to implement getConnection(username, password) according to my interpretation you'd need to add one level of indirection.

List<Connection> idleConnections = ...;

public Connection getConnection()
{
  return idleConnections.remove(nextConnection);
}

would become:

Map<String, List<Connection>> usernameToIdleConnections = ...;

public Connection getConnection()
{
  return usernmeToIdleConnections.get(username).remove(nextConnection);
}

If you do that, the rest of questions/problems you brought up melt away.

Does that make sense?

cowwoc commented Jan 20, 2015

@brettwooldridge My understanding of getConnection(username, password) is that the returned connection is meant to belong to the same pool (as opposed to spawning a separate pool as you are expecting).

Right now when a user requests a connection, you grab one from a list of idle connections. In order to implement getConnection(username, password) according to my interpretation you'd need to add one level of indirection.

List<Connection> idleConnections = ...;

public Connection getConnection()
{
  return idleConnections.remove(nextConnection);
}

would become:

Map<String, List<Connection>> usernameToIdleConnections = ...;

public Connection getConnection()
{
  return usernmeToIdleConnections.get(username).remove(nextConnection);
}

If you do that, the rest of questions/problems you brought up melt away.

Does that make sense?

brettwooldridge added a commit that referenced this issue Jan 20, 2015

Fix #231 Make the copyState() method public to facilitate wrappers wh…
…o want to copy/modify the configuration for use in multiple pool instances.
@chrisvest

This comment has been minimized.

Show comment
Hide comment
@chrisvest

chrisvest Jan 20, 2015

The getConnection(username, password) is fundamentally a caching API, rather than a pooling API, because it suddenly matters which connection you get. It is more or less a Map<Key,List<Connection>> with some replacement algorithm on the side. It's a rather different thing than a pool, though, and the performance characteristics will be different as well. For instance, a cache can't easily fill itself up automatically on initialisation, because it doesn't know which specific elements it will be asked for. For the same reason, requestor threads will most likely have to handle their own cache misses (like page faults), which badly influences tail latency (or worse, if the cache is too small).

chrisvest commented Jan 20, 2015

The getConnection(username, password) is fundamentally a caching API, rather than a pooling API, because it suddenly matters which connection you get. It is more or less a Map<Key,List<Connection>> with some replacement algorithm on the side. It's a rather different thing than a pool, though, and the performance characteristics will be different as well. For instance, a cache can't easily fill itself up automatically on initialisation, because it doesn't know which specific elements it will be asked for. For the same reason, requestor threads will most likely have to handle their own cache misses (like page faults), which badly influences tail latency (or worse, if the cache is too small).

@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge
Owner

brettwooldridge commented Jan 20, 2015

@chrisvest amen, brother

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 20, 2015

So you are advocating failing-fast on a function because it might result in less-than-desirable performance? (A theoretical problem that hasn't actually been measured)

That sounds a bit aggressive.

It is true that the pool can't fill itself automatically on initialization, but that is a drop in the bucket performance-wise. Pool initialization only happens once in an application's lifetime. Who says that performance beyond that point is noticeably slower than the normal getConnection() case? It would be great to have some concrete benchmark results to work against.

Anyway, I suggest adding a HikariDataSource copy constructor and moving on from there.

cowwoc commented Jan 20, 2015

So you are advocating failing-fast on a function because it might result in less-than-desirable performance? (A theoretical problem that hasn't actually been measured)

That sounds a bit aggressive.

It is true that the pool can't fill itself automatically on initialization, but that is a drop in the bucket performance-wise. Pool initialization only happens once in an application's lifetime. Who says that performance beyond that point is noticeably slower than the normal getConnection() case? It would be great to have some concrete benchmark results to work against.

Anyway, I suggest adding a HikariDataSource copy constructor and moving on from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment