chore: add a system property to set session pool ordering #2656
Conversation
Adds support for using a System property to set the default session pool ordering. The default ordering is LIFO. This can be changed to FIFO and RANDOM for users who need/want that.
|
|
||
| private static Position getReleaseToPositionFromSystemProperty() { | ||
| // NOTE: This System property is a beta feature. Support for it can be removed in the future. | ||
| String key = "SESSION_POOL_RELEASE_TO_POSITION"; |
There was a problem hiding this comment.
Maybe namespace this as com.google.cloud.spanner.session_pool_release_to_position?
There was a problem hiding this comment.
Yes, that is more idiomatic for System properties. All caps is for environment variables.
|
|
||
| private static Position getReleaseToPositionFromSystemProperty() { | ||
| // NOTE: This System property is a beta feature. Support for it can be removed in the future. | ||
| String key = "SESSION_POOL_RELEASE_TO_POSITION"; |
There was a problem hiding this comment.
Should we name this SPANNER_SESSION_POOL_RELEASE_TO_POSITION so that its easy for customers to identify that this one is being used for Spanner?
There was a problem hiding this comment.
As discussed offline, we'll go for the namespaced name com.google.cloud.spanner.session_pool_release_to_position.
| Session session3 = pool.getSession().get(); | ||
| Session session4 = pool.getSession().get(); | ||
| assertEquals(session2, session3); | ||
| assertEquals(session4, session1); |
There was a problem hiding this comment.
Nit: swap the expected/actual value
| assertEquals(session4, session1); | |
| assertEquals(session1, session4); |
| .collect(Collectors.toList()); | ||
| switch (position) { | ||
| case FIRST: | ||
| // FIFO: |
There was a problem hiding this comment.
Isn't this LIFO? Since the last element N will be added to the first, and we always poll from the front, we would always have the last one polled first.
There was a problem hiding this comment.
Eh, yes. Hence my comment about the naming being a bit confusing, I managed to confuse myself as well here.
| assertEquals(firstTime, Lists.reverse(secondTime)); | ||
| break; | ||
| case LAST: | ||
| // LIFO: |
There was a problem hiding this comment.
Similarly isn't this FIFO? Given everything gets added to the end of the queue, whatever was added first, will be removed first. No?
| String key = "SESSION_POOL_RELEASE_TO_POSITION"; | ||
| if (System.getProperties().containsKey(key)) { | ||
| try { | ||
| return Position.valueOf(System.getProperty(key).toUpperCase(Locale.ENGLISH)); |
There was a problem hiding this comment.
Would it be useful to add some logging for the case when this environment variable is set? Just so that if we run into any un-intended cases where a few other customers discover this option and set this, we at-least know if this option got set?
Logging is the minimum we can do, apart from adding to the trace?
There was a problem hiding this comment.
We wouldn't be able to see the logging, except if we add a gRPC header, and I don't think it's worth the latter. I don't worry about anyone finding and enabling this option on their own (and if they do, they probably also turn it off if they feel that it does not work well for them).
|
(retroactive LGTM from me as well) |
Adds support for using a System property to set the default session pool ordering. The default ordering is LIFO. This can be changed to FIFO and RANDOM for users who need/want that.