Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15235 )
Change subject: IMPALA-9152: Make Impala ranger plugin singleton in FE Tests ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/15235/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaPlugin.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaPlugin.java: http://gerrit.cloudera.org:8080/#/c/15235/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaPlugin.java@30 PS2, Line 30: RangerImpalaPlugin As far as I know (I am not a java expert) INSTANCE should be voletile for double-checked locking to work for singletons. http://gerrit.cloudera.org:8080/#/c/15235/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaPlugin.java@39 PS2, Line 39: Preconditions.checkState(SERVICE_TYPE == null || SERVICE_TYPE.equals(serviceType), : String.format("%s != %s", SERVICE_TYPE, serviceType)); : Preconditions.checkState(APP_ID == null || APP_ID.equals(appId), : String.format("%s != %s", APP_ID, appId)); nit: This could be skipped if there are two getInstance() calls in parallel with different configs and INSTANCE is still null. Moving the tests to the end seem more logical to me. -- To view, visit http://gerrit.cloudera.org:8080/15235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91f2bad1a9ce897b45cfc42f97b192fe2f8c7e06 Gerrit-Change-Number: 15235 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Tue, 18 Feb 2020 10:08:27 +0000 Gerrit-HasComments: Yes
