Re: [PR] optimize(api): disable GraphSpaceAPI and ManagerAPI in standalone mode [hugegraph]

[via GitHub]

imbajin commented on code in PR #2966:
URL: https://github.com/apache/hugegraph/pull/2966#discussion_r2936755627


##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/API.java:
##########
@@ -241,6 +241,13 @@ public static boolean checkAndParseAction(String action) {
         }

Review Comment:
   ⚠️ 
Suggestion: 
rename 
checkPdModeEnabled 
to a 
clearer name 
and add 
null-check
   
   This method 
throws when 
PD mode 
is not 
enabled. 
Rename to 
make the 
contract explicit 
(for 
example 
ensurePdModeEnabled 
or 
requirePdModeEnabled) 
and add 
a null-check 
+ [38;5
 ;204mJavadoc. 
Suggested 
change:
   
   public static 
void 
ensurePdModeEnabled(GraphManager
 manager) {
       
E.checkArgumentNotNull(manager,
 "G[3
 
8;5;198;48;5;233mraphManager
 
must
 
not
 be 
null");
       
if 
(!manager.usePD())
 {
           
throw
 
new
 
HugeException("GraphSpac[38;5;198;4
 8;5;233me 
management
 is 
not
 
supported
 in 
stan[38;5;198;48;
 
5;233mdalone
 
mode");
       }
   }
   
   Then update 
call sites 
(GraphSpaceAPI, 
ManagerAPI) 
to call 
ensurePdModeEnabled(manager).



##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -276,7 +276,7 @@ private static String serviceId(String graphSpace, 
Service.ServiceType type,
                      .replace("_", "-").toLowerCase();

Review Comment:
   ⚠️ 
Suggestion: 
rename public 
boolean 
usePD() 
to follow 
Java boolean 
getter 
conventions and 
add Javadoc
   
   Publicly exposing 
usePD() 
increases the 
class 
surface; 
prefer a 
clearer name 
such as 
isPdEnabled() 
and add 
javadoc 
describing PD 
semantics. 
Example:
   
   /** Returns true if placement driver (PD / hstore) is 
configured/available. */
   public boolean 
isPdEnabled() 
{
       
return
 
this.PDExist;
   }
   
   If callers 
outside core 
need the 
current 
name, 
keep 
usePD() 
as a 
deprecated 
delegate that 
calls the 
new method.



##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/API.java:
##########
@@ -241,6 +241,13 @@ public static boolean checkAndParseAction(String action) {
         }
     }
 

Review Comment:
   🧹 
Minor: 
Extract the 
literal error 
message into 
a constant 
to avoid 
duplication and 
typos. 
For example:
   
   private static 
final String 
STANDALONE_GRAPHSPACE_ERROR 
= "GraphSpace 
management is not supported in standalone 
mode";
   
   Then throw 
new 
HugeException(STANDALONE_GRAPHSPACE_ERROR);
 and update 
tests to 
reference the 
constant where 
practical.



##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/BaseApiTest.java:
##########
@@ -748,6 +739,28 @@ public static RestClient analystClient(String graphSpace, 
String username) {
         return analystClient;
     }
 
+    /**
+     * Skips the current test if the server is running in hstore / PD mode.
+     * Treats both {@code "hstore"} and {@code null} (i.e. the property is not
+     * set, which is the default in hstore CI runs) as PD mode.
+     * Call this from a {@code @Before} method in standalone-only test classes.
+     */
+    public static void assumeStandaloneMode() {
+        String backend = System.getProperty("backend");

Review Comment:
   ⚠️ 
Suggestion: 
clarify and 
simplify 
assumeStandaloneMode()
   
   The current 
condition 
`Assume.assumeTrue(backend != null && 
!backend.equals("hstore"))` 
is a 
double-negative 
and can 
be hard 
to 
read. 
Consider expressing 
the skip 
condition 
explicitly with 
`assumeFalse` 
and avoiding[39
 m NPEs. 
For example:
   
   public static 
void 
assumeStandaloneMode()
 {
       String 
backend = 
System.getProperty("backend");
       boolean 
isPdMode = 
backend == 
null || 
"hstore".equals(backend);
       
Assume.assumeFalse("skip
 standalone tests: backend='" 
+ backend 
+ "' (hstore/PD 
mode)", 
isPdMode);
   }
   
   This makes 
the intent 
clearer and 
avoids null 
checks in 
the boolean 
expression.



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hugegraph.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@hugegraph.apache.org
For additional commands, e-mail: issues-h...@hugegraph.apache.org