[fix] Modify the startup script to fix the log and protobuf dependenc…#5
[fix] Modify the startup script to fix the log and protobuf dependenc…#5kenssa4eedfd wants to merge 4 commits intohubble2from
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough新增对 Changes
Sequence Diagram(s)sequenceDiagram
participant Starter as start-hubble.sh
participant FS as 文件系统
participant CL as ClasspathBuilder
participant JVM as Java JVM
Note over Starter,FS `#DDEEFF`: 启动脚本：protobuf 专门解析与过滤
Starter->>FS: 搜索 LIB_PATH 中 protobuf-java-*.jar
alt 找到 PROTO_JAR
FS-->>Starter: 返回 PROTO_JAR 路径
Starter->>CL: 初始 CLASSPATH = ".:${PROTO_JAR}"
else 未找到
FS-->>Starter: 无匹配
Starter-->>Starter: 退出并报错
end
Starter->>FS: 遍历 LIB_PATH/*.jar（过滤日志绑定 & 跳过 PROTO_JAR）
FS-->>CL: 追加合格的 jars 到 CLASSPATH
CL->>JVM: 使用构建的 CLASSPATH 启动 Java 应用
sequenceDiagram
participant Caller as 使用者
participant Builder as HugeClientBuilder
participant Validator as 校验逻辑
Note over Caller,Builder `#E8F6E8`: 客户端构建：新增 graph 非空校验
Caller->>Builder: new HugeClientBuilder(hosts, port, graph)
Builder->>Validator: 检查 graph 非空且非空字符串
alt graph 有效
Validator-->>Builder: 通过
Builder->>Caller: 返回 Builder 实例
Caller->>Builder: build()
Builder->>Validator: 再次检查 graph 非空
Validator-->>Builder: 通过
Builder-->>Caller: 返回 HugeClient
else graph 为空/无效
Validator-->>Builder: 抛出错误并终止
Builder-->>Caller: 抛出异常
end
预估代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟
兔子之诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @kenssa4eedfd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness of the HugeGraph-Hubble startup process by rectifying critical dependency conflicts. The changes ensure that the application's logging and Protobuf components initialize without issues, leading to a more stable and predictable runtime environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the start-hubble.sh script to address dependency conflicts with protobuf and logging libraries. The changes ensure that a specific protobuf-java jar is prioritized in the classpath and that conflicting SLF4J logging bindings are excluded. My review includes a couple of suggestions to improve the script's efficiency and robustness. Overall, the changes are a good improvement.
| PROTO_JAR="" | ||
| for jar in "${LIB_PATH}"/protobuf-java-*.jar; do | ||
| [[ -e "$jar" ]] || break | ||
| PROTO_JAR="$jar" |
There was a problem hiding this comment.
This loop iterates over all matching protobuf-java-*.jar files and assigns the last one to PROTO_JAR. It's more efficient to stop after finding the first one. You can add a break to exit the loop immediately after a match is found. This also makes the behavior more predictable if multiple versions of the jar exist, by always selecting the first one in lexicographical order.
| PROTO_JAR="$jar" | |
| PROTO_JAR="$jar"; break |
| base=$(basename "$jar") | ||
| # Avoid multiple slf4j bindings: keep log4j-slf4j-impl, drop log4j-to-slf4j and logback | ||
| case "${base}" in | ||
| log4j-to-slf4j-*.jar|logback-classic-*.jar|logback-core-*.jar) | ||
| continue | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Calling basename inside a loop is inefficient as it starts a new process on each iteration. This can be optimized by using shell's built-in glob matching within the case statement, which avoids forking a process.
| base=$(basename "$jar") | |
| # Avoid multiple slf4j bindings: keep log4j-slf4j-impl, drop log4j-to-slf4j and logback | |
| case "${base}" in | |
| log4j-to-slf4j-*.jar|logback-classic-*.jar|logback-core-*.jar) | |
| continue | |
| ;; | |
| esac | |
| # Avoid multiple slf4j bindings: keep log4j-slf4j-impl, drop log4j-to-slf4j and logback | |
| case "$jar" in | |
| */log4j-to-slf4j-*.jar|*/logback-classic-*.jar|*/logback-core-*.jar) | |
| continue | |
| ;; | |
| esac |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@codecov-ai-reviewer review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java (1)
89-92: 建议在configGraph()中添加参数校验。
configGraph()方法允许在构造后设置graph参数，但没有进行任何校验。这会破坏构造器建立的不变性约束（graph 必须非空且非空字符串）。调用者可以通过configGraph(null)或configGraph("")绕过构造器的校验。建议添加与构造器一致的校验逻辑：
public HugeClientBuilder configGraph(String graph) { + E.checkArgument(graph != null && !graph.isEmpty(), + "Expect a string value as the graph name parameter argument, but got: %s", + graph); this.graph = graph; return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: hubble-ci (11, 3.11)
- GitHub Check: check-dependency-license
- GitHub Check: loader-ci (11)
- GitHub Check: client-ci (11)
- GitHub Check: tools-ci (11)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (java)
- GitHub Check: spark-connector-ci (11)
🔇 Additional comments (1)
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java (1)
57-59: 做得好！添加了防御性参数校验。在构造器中对
graph参数进行非空且非空字符串的校验是良好的防御性编程实践，可以尽早捕获无效输入。错误消息清晰且与现有的url参数校验保持一致。
|
|
||
| public HugeClient build() { | ||
| E.checkArgument(this.url != null, "The url parameter can't be null"); | ||
| E.checkArgument(this.graph != null, "The graph parameter can't be null"); |
There was a problem hiding this comment.
校验逻辑与构造器不一致。
构造器（第 57-59 行）校验 graph 既不为 null 也不为空字符串，但此处仅校验非 null。如果在构造后调用 configGraph("")，build() 方法不会捕获空字符串，导致校验逻辑不一致。
建议应用以下修复：
- E.checkArgument(this.graph != null, "The graph parameter can't be null");
+ E.checkArgument(this.graph != null && !this.graph.isEmpty(),
+ "The graph parameter can't be null or empty");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| E.checkArgument(this.graph != null, "The graph parameter can't be null"); | |
| E.checkArgument(this.graph != null && !this.graph.isEmpty(), | |
| "The graph parameter can't be null or empty"); |
🤖 Prompt for AI Agents
In
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClientBuilder.java
around line 80, the build() method only checks graph != null but the constructor
validated graph is neither null nor empty; update the check to require the graph
be non-null and not empty (preferably after trimming) so build() rejects
empty-string graphs the same way as the constructor, and update the error
message accordingly.
|
Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label |
…y conflict
Purpose of the PR
Main Changes
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedSummary by CodeRabbit
缺陷修复
行为变更
✏️ Tip: You can customize this high-level summary in your review settings.