[GitHub] [shardingsphere] tristaZero commented on a change in pull request #6359: HintManager's getInstance returns the instance held by the current th…

[GitBox <gi...@apache.org>]

tristaZero commented on a change in pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#discussion_r456193433



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/hint/HintManager.java
##########
@@ -43,15 +42,20 @@
     private boolean masterRouteOnly;
     
     /**
-     * Get a new instance for {@code HintManager}.
+     * Get a  instance for {@code HintManager}.
      *
-     * @return  {@code HintManager} instance
+     * @return  {@code HintManager}  current thread hold or new instance
      */
     public static HintManager getInstance() {
-        Preconditions.checkState(null == HINT_MANAGER_HOLDER.get(), "Hint has previous value, please clear first.");
-        HintManager result = new HintManager();
-        HINT_MANAGER_HOLDER.set(result);
-        return result;
+
+        final HintManager hintManager = HINT_MANAGER_HOLDER.get();

Review comment:
       `final` is necessary or not? If it is optional, please remove it.

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/hint/HintManager.java
##########
@@ -43,15 +42,20 @@
     private boolean masterRouteOnly;
     
     /**
-     * Get a new instance for {@code HintManager}.
+     * Get a  instance for {@code HintManager}.
      *
-     * @return  {@code HintManager} instance
+     * @return  {@code HintManager}  current thread hold or new instance
      */
     public static HintManager getInstance() {
-        Preconditions.checkState(null == HINT_MANAGER_HOLDER.get(), "Hint has previous value, please clear first.");
-        HintManager result = new HintManager();
-        HINT_MANAGER_HOLDER.set(result);
-        return result;
+

Review comment:
       Invalid blank line, please remove.

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/hint/HintManagerTest.java
##########
@@ -20,17 +20,23 @@
 import org.junit.Test;
 
 import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 public final class HintManagerTest {
-    
-    @Test(expected = IllegalStateException.class)
+
+    @Test
     public void assertGetInstanceTwice() {
         try {
-            HintManager.getInstance();
-            HintManager.getInstance();
+            final HintManager instance = HintManager.getInstance();

Review comment:
       final is necessary or not? If it is optional, please remove it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org