AlexStocks commented on code in PR #3502: URL: https://github.com/apache/dubbo-go/pull/3502#discussion_r3699001707
########## tools/benchmark/scripts/run_single.sh: ########## @@ -0,0 +1,134 @@ +#!/bin/bash +# +# 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. +# + +set -e + +BASE_DIR=$(cd "$(dirname "$0")/.." && pwd) +LOG_DIR="$BASE_DIR/logs" +SEPARATOR="========================================" + +mkdir -p "$LOG_DIR" + +FRAMEWORK="${1:-dubbo-go}" +PAYLOAD="${2:-1024}" +SERIALIZATION="${3:-protobuf}" +COMPRESSION="${4:-none}" +CONCURRENCY="${5:-100}" +CALL_MODE="${6:-unary}" + +wait_for_port() { + local port=$1 + local timeout=${2:-30} + local elapsed=0 + while [ $elapsed -lt $timeout ]; do + if nc -z localhost "$port" 2>/dev/null; then + return 0 + fi + sleep 1 + elapsed=$((elapsed + 1)) + done + return 1 +} + +echo "$SEPARATOR" +echo " Dubbo-Go Benchmark - Single Test" +echo "$SEPARATOR" +echo "Framework: $FRAMEWORK" +echo "Payload Size: $PAYLOAD bytes" +echo "Serialization: $SERIALIZATION" +echo "Compression: $COMPRESSION" +echo "Concurrency: $CONCURRENCY" +echo "Call Mode: $CALL_MODE" +echo "$SEPARATOR" + +echo "[INFO] Compiling server..." +case "$FRAMEWORK" in + dubbo-go) + cd "$BASE_DIR/server/dubbo-go" + go build -o benchmark-dubbo-go main.go + SERVER_BIN="$BASE_DIR/server/dubbo-go/benchmark-dubbo-go" + SERVER_PORT=20000 + ;; + grpc) + cd "$BASE_DIR/server/grpc" + go build -o benchmark-grpc main.go + SERVER_BIN="$BASE_DIR/server/grpc/benchmark-grpc" + SERVER_PORT=50051 + ;; + *) + echo "[ERROR] Unsupported framework: $FRAMEWORK" + exit 1 + ;; +esac + +echo "[INFO] Compiling client..." +cd "$BASE_DIR/client" +go build -o benchmark-client main.go + +LOG_FILE="$LOG_DIR/${FRAMEWORK}_${PAYLOAD}_${SERIALIZATION}_${COMPRESSION}_${CONCURRENCY}_${CALL_MODE}.log" + +echo "" +echo "[INFO] Starting server..." +case "$FRAMEWORK" in + dubbo-go) + "$SERVER_BIN" --serialization "$SERIALIZATION" --compression "$COMPRESSION" --port "$SERVER_PORT" > "$LOG_FILE.server.log" 2>&1 & + ;; + grpc) + "$SERVER_BIN" --port "$SERVER_PORT" > "$LOG_FILE.server.log" 2>&1 & + ;; + *) + echo "[ERROR] Unsupported framework: $FRAMEWORK" + exit 1 + ;; +esac +pid=$! Review Comment: [P1] 获取 PID 后立即注册失败和信号清理 脚本启用了 `set -e`，但只有客户端成功返回后才在第 123 行停止 server。客户端连接失败、参数错误、结果写入失败或用户按 Ctrl-C 时都会提前退出并跳过清理，留下 benchmark server 和占用中的 20000/50051 端口，后续用例可能连接到残留进程。 请在 `pid=$!` 后立即注册幂等的 `trap cleanup EXIT INT TERM`，按 TERM、有限等待、必要时 KILL、`wait` 的顺序回收；补客户端非零退出和 SIGINT 两条脚本集成测试。 ########## tools/benchmark/scripts/gen_code.sh: ########## @@ -0,0 +1,48 @@ +#!/bin/bash +# +# 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. +# + +set -e + +BASE_DIR=$(cd "$(dirname "$0")/.." && pwd) +PROJECT_ROOT=$(cd "$BASE_DIR/../.." && pwd) +PROTO_DIR="$BASE_DIR/proto" +OUT_DIR="$PROTO_DIR" +PLUGIN_DIR="$PROJECT_ROOT/tools/protoc-gen-go-triple" + +mkdir -p "$OUT_DIR" + +echo "[INFO] Building protoc-gen-go-triple plugin..." +cd "$PLUGIN_DIR" +go build -o protoc-gen-go-triple . +cd - + +echo "[INFO] Generating protobuf code..." +protoc --proto_path="$PROTO_DIR" --go_out="$OUT_DIR" --go_opt=paths=source_relative "benchmark.proto" Review Comment: [P1] 同步生成 gRPC stub README 声称此脚本会生成 `benchmark_grpc.pb.go`，但这里仅执行 `--go_out`，后续也只有 `--go-triple_out`，没有调用 `protoc-gen-go-grpc`。修改 `benchmark.proto` 的 RPC 后运行脚本会更新 message 和 Triple stub，却保留陈旧的 gRPC client/server stub，可能继续测试旧 API 或直接构建失败。 请校验固定版本的 `protoc-gen-go-grpc`，增加 `--go-grpc_out`/`paths=source_relative`，并在临时副本中执行生成后对三份生成文件做零 Diff 验证。 ########## tools/benchmark/scripts/gen_code.sh: ########## @@ -0,0 +1,48 @@ +#!/bin/bash +# +# 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. +# + +set -e + +BASE_DIR=$(cd "$(dirname "$0")/.." && pwd) +PROJECT_ROOT=$(cd "$BASE_DIR/../.." && pwd) +PROTO_DIR="$BASE_DIR/proto" +OUT_DIR="$PROTO_DIR" +PLUGIN_DIR="$PROJECT_ROOT/tools/protoc-gen-go-triple" + +mkdir -p "$OUT_DIR" + +echo "[INFO] Building protoc-gen-go-triple plugin..." +cd "$PLUGIN_DIR" +go build -o protoc-gen-go-triple . Review Comment: [P1] 不要在工具源码目录覆盖临时插件 这里把构建产物写到 `tools/protoc-gen-go-triple/protoc-gen-go-triple`。后续任一 `protoc` 命令失败时，`set -e` 会在第 45 行 cleanup 前退出并遗留二进制；如果调用前已有同名用户构建产物，还会先覆盖它，成功后再无条件删除。 请使用 `mktemp -d` 创建每次运行唯一的临时目录，并在创建后立即注册幂等 trap；`protoc --plugin` 应指向该临时路径，不能覆盖或删除工具源码目录中的既有文件。 ########## tools/benchmark/scripts/run_all.sh: ########## @@ -0,0 +1,189 @@ +#!/bin/bash +# +# 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. +# + +set -e + +BASE_DIR=$(cd "$(dirname "$0")/.." && pwd) +LOG_DIR="$BASE_DIR/logs" +REPORT_DIR="$BASE_DIR/report" +DATA_DIR="$BASE_DIR/data" +PID_FILE="/tmp/benchmark_server.pid" Review Comment: [P1] PID 文件必须按脚本实例隔离 所有运行共享固定的 `/tmp/benchmark_server.pid`。并发启动 A/B 时，B 会覆盖 A 的 PID；任一实例触发 EXIT cleanup 后都可能终止另一个实例，PID 重用窗口内还可能误杀无关进程，benchmark 数据也会互相污染。 请直接由当前 shell 持有子进程 PID；若必须落盘，使用 `mktemp` 创建每实例唯一文件，并在 kill 前核对 PID 对应的启动时间或可执行文件身份。请补两个并发脚本实例互不终止对方的测试。 ########## tools/benchmark/client/payload/payload.go: ########## @@ -0,0 +1,63 @@ +/* + * 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 payload + +import ( + "crypto/rand" + "sync" +) + +type PayloadGenerator struct { + payloads sync.Map +} + +func NewPayloadGenerator() *PayloadGenerator { + return &PayloadGenerator{} +} + +func (pg *PayloadGenerator) Generate(size int) []byte { + if cached, ok := pg.payloads.Load(size); ok { + return cached.([]byte) + } + + data := make([]byte, size) Review Comment: [P1] 在分配前限制 payload 和其他数值参数 `--payload` 未校验就进入 `make`：负数会稳定 panic，任意超大值可在建立连接前耗尽内存；`concurrency <= 0` 等无效参数还会生成零请求但看似成功的 benchmark 结果。 请在创建 payload 和 Engine 前统一验证 payload、concurrency、duration/warmup/timeout 以及 framework/serialization/compression/mode 枚举，并设置与实际协议限制一致的上限。测试至少覆盖 `-1`、`0`、1 MiB、16 MiB 和超限值。 -- 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]
