[SQL] add note of use synchronizedMap in SQLConf #1996
Conversation
|
Can one of the admins verify this patch? |
|
That winning answer on SO has no clue about performance. In most cases with low degree of contention, ConcurrentHashMap is both slower and uses more memory. i.e. we should close this PR. Thanks. |
|
Actually to prevent this from happening in the future, do you mind change the PR to add a line above the declaration to state "Only low degree of contention is expected for conf, thus NOT using ConcurrentHashMap" ? |
|
The only way you argued me out of this last time @rxin was that you said we'd have one of these per thread. Is that still happening? |
|
It happens for the main use case which is the server. And for non-server, users have to explicitly create multiple threads to access conf to make contention happen. Either way, this code path is not some highly contended thing that you'd want a ConcurrentHashMap for. |
scwf
changed the title
|
@rxin , thanks, updated. |
|
Jenkins, test this please. |
|
QA tests have started for PR 1996 at commit
|
|
QA tests have finished for PR 1996 at commit
|
|
Jenkins, retest this please. |
|
QA tests have started for PR 1996 at commit
|
|
QA tests have finished for PR 1996 at commit
|
|
Jenkins, retest this please. |
|
QA tests have started for PR 1996 at commit
|
|
QA tests have finished for PR 1996 at commit
|
|
There are something wrong with CliSuite and HiveThriftServer2Suite, the master branch can not pass this two tests. refer to #2036 |
|
I'm going to merge this since the test failures are expected. |
Refer to: http://stackoverflow.com/questions/510632/whats-the-difference-between-concurrenthashmap-and-collections-synchronizedmap Collections.synchronizedMap(map) creates a blocking Map which will degrade performance, albeit ensure consistency. So use ConcurrentHashMap(a more effective thread-safe hashmap) instead. also update HiveQuerySuite to fix test error when changed to ConcurrentHashMap. Author: wangfei <[email protected]> Author: scwf <[email protected]> Closes #1996 from scwf/sqlconf and squashes the following commits: 93bc0c5 [wangfei] revert change of HiveQuerySuite 0cc05dd [wangfei] add note for use synchronizedMap 3c224d3 [scwf] fix formate a7bcb98 [scwf] use ConcurrentHashMap in sql conf, intead synchronizedMap (cherry picked from commit 0e3ab94) Signed-off-by: Reynold Xin <[email protected]>
Refer to: http://stackoverflow.com/questions/510632/whats-the-difference-between-concurrenthashmap-and-collections-synchronizedmap Collections.synchronizedMap(map) creates a blocking Map which will degrade performance, albeit ensure consistency. So use ConcurrentHashMap(a more effective thread-safe hashmap) instead. also update HiveQuerySuite to fix test error when changed to ConcurrentHashMap. Author: wangfei <[email protected]> Author: scwf <[email protected]> Closes apache#1996 from scwf/sqlconf and squashes the following commits: 93bc0c5 [wangfei] revert change of HiveQuerySuite 0cc05dd [wangfei] add note for use synchronizedMap 3c224d3 [scwf] fix formate a7bcb98 [scwf] use ConcurrentHashMap in sql conf, intead synchronizedMap
Refer to:
http://stackoverflow.com/questions/510632/whats-the-difference-between-concurrenthashmap-and-collections-synchronizedmap
Collections.synchronizedMap(map) creates a blocking Map which will degrade performance, albeit ensure consistency. So use ConcurrentHashMap(a more effective thread-safe hashmap) instead.
also update HiveQuerySuite to fix test error when changed to ConcurrentHashMap.