[
https://issues.apache.org/jira/browse/PHOENIX-901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13951476#comment-13951476
]
Lars Hofhansl commented on PHOENIX-901:
---------------------------------------
So looking at all the complexity here, I think is far better to turn
connectionQueryServicesMap into a normal map, not synchronizing in the init()
method, and just add synchronized to getConnectionQueryServices (maybe add a
synchronized(this) inside to let the first few instruction run unsynchronized).
Here's why:
# The main problem is that we need to make creation of the object and placement
of the object into map atomic if initialization of the object is potentially
non-trivial (as is the case here).
# If multiple threads come in the first time, we want them to wait anyway,
unless one succeeds to initialize the connection
# Once the connection is initialized it is just a lookup in a map. So each
thread would leave the synchronized section after a couple of nanoseconds.
# a volatile is roughly as expensive as an uncontended synchronized (the cost
is setting up the memory fences)
# getConnectionQueryServices won't be called more than once per query
(correct?), so it's not on the critical path
# It's easy to follow and easy to understand
> Ensure ConnectionQueryServices only initialized once
> ----------------------------------------------------
>
> Key: PHOENIX-901
> URL: https://issues.apache.org/jira/browse/PHOENIX-901
> Project: Phoenix
> Issue Type: Bug
> Reporter: James Taylor
> Assignee: James Taylor
> Fix For: 3.0.0, 4.0.0
>
> Attachments: phoenix.patch, phoenix_2.patch, phoenix_3.patch,
> phoenix_4.patch
>
>
> We should call connectionQueryServices#init in the else block of this code:
> {code}
> @Override
> protected ConnectionQueryServices getConnectionQueryServices(String url,
> Properties info) throws SQLException {
> checkClosed();
> ConnectionInfo connInfo = ConnectionInfo.create(url);
> ConnectionInfo normalizedConnInfo =
> connInfo.normalize(getQueryServices().getProps());
> ConnectionQueryServices connectionQueryServices =
> connectionQueryServicesMap.get(normalizedConnInfo);
> if (connectionQueryServices == null) {
> if (normalizedConnInfo.isConnectionless()) {
> connectionQueryServices = new
> ConnectionlessQueryServicesImpl(getQueryServices());
> } else {
> connectionQueryServices = new
> ConnectionQueryServicesImpl(getQueryServices(), normalizedConnInfo);
> }
> connectionQueryServices.init(url, info);
> ConnectionQueryServices prevValue =
> connectionQueryServicesMap.putIfAbsent(normalizedConnInfo,
> connectionQueryServices);
> if (prevValue != null) {
> connectionQueryServices = prevValue;
> }
> }
> return connectionQueryServices;
> }
> {code}
> like this instead:
> {code}
> @Override
> protected ConnectionQueryServices getConnectionQueryServices(String url,
> Properties info) throws SQLException {
> checkClosed();
> ConnectionInfo connInfo = ConnectionInfo.create(url);
> ConnectionInfo normalizedConnInfo =
> connInfo.normalize(getQueryServices().getProps());
> ConnectionQueryServices connectionQueryServices =
> connectionQueryServicesMap.get(normalizedConnInfo);
> if (connectionQueryServices == null) {
> if (normalizedConnInfo.isConnectionless()) {
> connectionQueryServices = new
> ConnectionlessQueryServicesImpl(getQueryServices());
> } else {
> connectionQueryServices = new
> ConnectionQueryServicesImpl(getQueryServices(), normalizedConnInfo);
> }
> ConnectionQueryServices prevValue =
> connectionQueryServicesMap.putIfAbsent(normalizedConnInfo,
> connectionQueryServices);
> if (prevValue != null) {
> connectionQueryServices = prevValue;
> } else {
> connectionQueryServices.init(url, info);
> }
> }
> return connectionQueryServices;
> }
> {code}
> This has the potential to open multiple HConnections, but it's unclear if
> this causes harm, as the same, original ConnectionQueryService is returned
> and used.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
