bitflicker64 commented on code in PR #2945:
URL: https://github.com/apache/hugegraph/pull/2945#discussion_r4252227482
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/arthas/ArthasAPI.java:
##########
@@ -49,13 +50,29 @@ public class ArthasAPI extends API {
@Operation(summary = "start arthas agent")
public Object startArthas() {
HugeConfig config = this.configProvider.get();
+ HashMap<String, String> configMap = arthasConfig(config);
+ ArthasAgent.attach(configMap);
+ return JsonUtil.toJson(configMap);
+ }
+
+ static HashMap<String, String> arthasConfig(HugeConfig config) {
HashMap<String, String> configMap = new HashMap<>(4);
- configMap.put("arthas.telnetPort",
config.get(ServerOptions.ARTHAS_TELNET_PORT));
- configMap.put("arthas.httpPort",
config.get(ServerOptions.ARTHAS_HTTP_PORT));
+ configMap.put("arthas.telnetPort",
+ option(config, ServerOptions.ARTHAS_TELNET_PORT,
+ ServerOptions.ARTHAS_TELNET_PORT_LEGACY));
+ configMap.put("arthas.httpPort",
+ option(config, ServerOptions.ARTHAS_HTTP_PORT,
+ ServerOptions.ARTHAS_HTTP_PORT_LEGACY));
configMap.put("arthas.ip", config.get(ServerOptions.ARTHAS_IP));
configMap.put("arthas.disabledCommands",
- config.get(ServerOptions.ARTHAS_DISABLED_COMMANDS));
- ArthasAgent.attach(configMap);
- return JsonUtil.toJson(configMap);
+ option(config, ServerOptions.ARTHAS_DISABLED_COMMANDS,
+ ServerOptions.ARTHAS_DISABLED_COMMANDS_LEGACY));
+ return configMap;
+ }
+
+ private static String option(HugeConfig config, ConfigOption<String>
canonical,
+ ConfigOption<String> legacy) {
+ return config.containsKey(canonical.name()) ? config.get(canonical) :
Review Comment:
Important: snake_case-first precedence silently drops a working camelCase
override on upgrade.
Since 1.2.0 the shipped `rest-server.properties` has carried
`arthas.telnet_port=8562`, `arthas.http_port=8561` and
`arthas.disabled_commands=jad`, while the code only read the camelCase keys. So
the only way to customise these values was to add `arthas.telnetPort` /
`arthas.httpPort` / `arthas.disabledCommands`, and the shipped snake_case lines
were inert and had no reason to be removed.
For that config (shipped snake_case defaults plus a camelCase override),
`containsKey(canonical.name())` is true here, so the default-valued snake_case
line now wins:
- `arthas.telnet_port=8562` + `arthas.telnetPort=9562`: master attaches on
9562, this head on 8562.
- `arthas.disabled_commands=jad` +
`arthas.disabledCommands=jad,ognl,vmtool`: master disables all three, this head
re-enables `ognl` and `vmtool`.
`testExplicitCanonicalDefaultsTakePrecedence` in the Server
`ArthasConfigTest` asserts exactly this outcome, so it is intended, but it
changes effective behaviour for an existing config without any opt-in, and
nothing is logged.
Requested change: on Server, let an explicitly configured legacy key win
when both forms are present (it was the only effective key before, so every
existing config keeps its behaviour, and a snake_case-only config still gets
fixed), or at minimum log a warning when both forms are set with different
values. Update the precedence note in `rest-server.properties` and the test to
match.
##########
hugegraph-store/hg-store-node/src/test/java/org/apache/hugegraph/store/node/ArthasConfigTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.store.node;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.core.env.MapPropertySource;
+import org.springframework.core.env.StandardEnvironment;
+
+public class ArthasConfigTest {
Review Comment:
Important: this test class is not run by any CI job.
`.github/workflows/pd-store-ci.yml` runs the `hg-store-node` module only as
`mvn test -pl hugegraph-store/hg-store-node -am -Dtest=StoredRowIngressTest
...` (line 417), and the `hg-store-test` suites exclude `**/node/**`. The
`pd_store / hstore` job log for this head has no `ArthasConfigTest` execution.
The `@Value` placeholder change in `AppConfig.ArthasConfig` is therefore
unverified in CI, and a later edit to those placeholders would not be caught.
Requested change: add the class to that step's selection, for example
`-Dtest='StoredRowIngressTest,ArthasConfigTest'`, so the alias and precedence
cases run on every PR.
--
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]
