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_r2936769177


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

Review Comment:
   Consider renaming 
`checkPdModeEnabled(GraphManager 
manager)`. 
The current 
name is 
non-idiomatic 
for a 
method that 
throws on 
failure; 
prefer a 
verb that 
indicates it 
asserts/ensures 
the [3
 9mcondition, 
e.g. 
`ensurePdModeEnabled(...)`
 or 
`requirePdModeEnabled(...)`.
   
   Also consider 
the following 
improvements:
   - Add 
a defensive 
null-check 
for the 
`manager` 
parameter, 
e.g. 
`E.checkArgumentNotNull(manager, "manager can't 
be null")` to 
avoid potential 
NPEs.
   - Add 
Javadoc describing 
the 
method's behaviour and the 
exception it throws (`HugeException` when PD mode is not enabled).



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

Review Comment:
   This helper 
is declared 
`public static` 
on the 
widely-used 
`API` 
base 
class, 
which unnecessarily 
expands the 
public API 
surface. 
If this 
method is 
only used [
 39mby API 
subclasses, 
consider reducing 
visibility 
(for 
example 
`protected` 
or 
package-private)
 so it 
is not 
exposed as 
part of 
the public 
API.



##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/API.java:
##########
@@ -241,6 +241,13 @@ public static boolean checkAndParseAction(String action) {
         }
     }
 
+    public static void checkPdModeEnabled(GraphManager manager) {
+        if (!manager.usePD()) {
+            throw new HugeException(
+                    "GraphSpace management is not supported in standalone 
mode");

Review Comment:
   The error 
message string 
`"GraphSpace 
management is not supported in standalone 
mode"` 
is duplicated 
between this 
code and 
the tests 
(e.g.
 GraphSpaceApiStandaloneTest 
/ 
ManagerApiStandaloneTest).
 Extract this 
literal
  into a 
single constant 
to avoid 
duplication and 
ensure 
consistency, 
for example:
   
   
```
   private static 
final String 
STANDALONE_ERROR =
           "GraphSpace management is 
not supported in standalone mode";
   
```
   
   If tests 
need to 
reference 
it, 
consider placing 
it in 
a shared 
test-friendly 
location or 
making it 
package-visible/public
 as 
appropriate.



##########
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();
     }
 
-    private boolean usePD() {
+    public boolean usePD() {

Review Comment:
   The method 
name 
`usePD()` 
is ambiguous 
for a 
boolean getter 
and does 
not follow 
standard Java 
boolean naming 
conventions. 
Consider renaming 
to 
`isPdEnabled()` 
(or `[38;5
 ;15misUsePD()`) 
for 
clarity, 
and review 
whether it 
needs to 
be 
`public` 
or can 
be made 
less visible 
if used 
only 
internally.



##########
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");
+        Assume.assumeTrue(
+                "skip standalone tests: backend is '" + backend + "' 
(hstore/PD mode)",
+                backend != null && !backend.equals("hstore"));

Review Comment:
   The current 
`assumeStandaloneMode()`
 uses a 
double-negative 
in the 
condition 
(`backend != null && 
!backend.equals("hstore")`),
 which reduces 
readability. 
For clarity 
and 
null-safety 
prefer an 
explicit bo
 olean variable 
and 
`Assume.assumeFalse`,
 for example:
   
   ```
   boolean isPdMode = backend == null || 
backend.equals("hstore");
   Assume.assumeFalse(
           "skip standalone tests: backend is '" + backend + "' 
(hstore/PD mode)",
           isPdMode);
   ```
   
   This makes 
the intent 
(`isPdMode`)
 explicit and 
avoids the 
double-negative.



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