bitflicker64 commented on code in PR #3391:
URL: https://github.com/apache/hugegraph/pull/3391#discussion_r4349470557
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/io/HugeGraphSONModule.java:
##########
@@ -369,6 +370,8 @@ public GraphSpace deserialize(JsonParser jsonParser,
memoryUsed = jsonParser.getNumberValue();
} else if ("storage_used".equals(fieldName)) {
storageUsed = jsonParser.getNumberValue();
+ } else if ("storage_used_kb".equals(fieldName)) {
+ storageUsedKb = jsonParser.getNumberValue();
Review Comment:
Important: Once a Server from before this PR rewrites a space record, this
branch can no longer read it, and `GraphManager` init fails.
The new `info()` always writes `storage_used_kb`. The old deserializer
stores unknown fields in `configs` via `getValueAsString()`, and the old
`info()` writes `configs` back. Old Servers rewrite the record on
`createGraph`, `dropGraph` and a space PUT, so the value becomes
`"storage_used_kb":"524288"` and this line throws `Current token (VALUE_STRING)
not numeric`. I reproduced it with master's and this branch's compiled
`hugegraph-core`. `graphSpaceConfigs()` has no per-entry catch, and both
`createDefaultGraphSpaceIfNeeded` and `loadGraphSpaces` call it, so one such
record stops the whole Server. A rolling `helm upgrade` of the 3-replica Server
Deployment can hit this, and so can a re-upgrade after `helm rollback`.
Fix: `storageUsedKb = jsonParser.getValueAsLong(-1);` (it also parses the
string form), plus a round-trip test.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java:
##########
@@ -999,31 +1002,26 @@ public Map getQuota() throws PDException {
storages.put(graphSpace, size);
}
- Metapb.GraphSpace.Builder spaceBuilder =
Metapb.GraphSpace.newBuilder();
HashMap<String, Boolean> limits = new HashMap<>();
for (Map.Entry<String, Long> item : storages.entrySet()) {
String spaceName = item.getKey();
+ long size = item.getValue();
String value = kvService.get(graphSpaceConfPrefix + spaceName);
+ boolean limited = false;
if (!StringUtils.isEmpty(value)) {
HashMap config = new Gson().fromJson(value, HashMap.class);
- Long size = item.getValue();
int limit = ((Double) config.get("storage_limit")).intValue();
- long limitByLong = limit * 1024L * 1024L;
- try {
-
spaceBuilder.setName(spaceName).setStorageLimit(limitByLong).setUsedSize(size);
- Metapb.GraphSpace graphSpace = spaceBuilder.build();
- configService.setGraphSpace(graphSpace);
- } catch (Exception e) {
- log.error("update graph space with error:", e);
- }
- // KB and GB * 1024L * 1024L
- if (size > limitByLong) {
- limits.put(spaceName, true);
- continue;
- }
+ // Store statistics and PD graph-space usage are in KB;
configuration is in GB.
+ long limitKb = limit * 1024L * 1024L;
+ limited = size > limitKb;
}
- limits.put(spaceName, false);
-
+ // Statistics must also advance when no Server config exists under
the default cluster.
+ try {
+ configService.setGraphSpaceUsage(spaceName, size);
Review Comment:
Minor: Records written by an older PD keep their limit in KB, so PD reports
mixed units after an upgrade.
The base `getQuota()` rewrote `storage_limit` as `limit * 1024 * 1024` every
cycle for each space with a Server config. This loop now writes only usage and
keeps the stored limit. So a 100 GB space carried over from the base still
shows `storage_limit: 104857600` in PD `GET /graph-spaces`, while new spaces
show 100. It stays that way until the space is next updated through the Server,
since every space PUT calls `limitStorage`. Enforcement does not read this
field, so only reporting is affected.
Please note in `docs/server-runtime.md` that one space PUT per space after
the PD upgrade rewrites the limit in GB.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -415,15 +415,11 @@ public Map<String, Object> manage(@Context GraphManager
manager,
exist.storageLimit = storageLimit;
}
- int computeCpuLimit = (int) graphSpaceMap
- .getOrDefault("compute_cpu_limit", 0);
- if (computeCpuLimit != 0) {
- exist.computeCpuLimit(computeCpuLimit);
+ if (graphSpaceMap.containsKey("compute_cpu_limit")) {
Review Comment:
Minor: The Java client always sends both compute fields, so a partial update
now resets explicit compute limits to inherit.
`GraphSpace.GraphSpaceReq` in hugegraph-client (toolchain,
`structure/space/GraphSpace.java`) copies `computeCpuLimit` and
`computeMemoryLimit` as `int`, with no inclusion filter. An update built
without them sends 0. Before this PR, 0 kept the stored value. Now
`containsKey` is true and a stored 16 becomes 0. Read-modify-write callers are
not affected, because GET returns the stored value. Apart from GET then
reporting 0, the only effect is under `server.use_k8s`, where the compute quota
falls back to `cpu_limit` and `memory_limit`.
Toolchain #834 fixes only Hubble (`GraphSpaceEntity.convertReq`). Moving
that omission into `GraphSpaceReq` in #834 covers both callers, and the two PRs
should then land together. Released clients still send 0, so please also note
this in the upgrade docs.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
