Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(driver): 使用绝对路径解决 Java 插件执行问题 #2995

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

littleniannian
Copy link
Collaborator

@littleniannian littleniannian commented Mar 26, 2025

User description

关联的 issue

https://github.com/actiontech/sqle-ee/issues/2307

描述你的变更

  • 新增 getJdkPath 函数获取 JDK 绝对路径
  • 更新 Java 插件执行命令,使用绝对路径
  • 移除 RPM 构建中对 SQLE_JAVA_HOME 环境变量的设置

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc


Description

  • 通过 getJdkPath 函数获取 JDK 绝对路径

  • 更新 Java 插件执行命令以使用绝对路径

  • 移除 RPM 构建中设置 SQLE_JAVA_HOME 环境变量

  • 添加错误处理以确保获取当前目录时的稳定性


Changes walkthrough 📝

Relevant files
Bug fix
plugin_manager.go
优化 Java 插件路径管理                                                                                     

sqle/driver/plugin_manager.go

  • 添加 getJdkPath 函数获取 JDK 路径
  • 更新 Java 插件命令使用绝对路径
  • 增加错误处理以确保跨平台兼容性
+12/-2   
Configuration changes
sqled.spec
调整 sqled.spec 构建脚本                                                                             

build/sqled.spec

  • 移除 SQLE_JAVA_HOME 环境变量设置
  • 更新 systemd 服务配置
+0/-1     
sqled_with_dms.spec
调整 sqled_with_dms.spec 构建脚本                                                           

build/sqled_with_dms.spec

  • 移除 SQLE_JAVA_HOME 环境变量设置
  • 更新 systemd 服务配置
+0/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • - 新增 getJdkPath 函数获取 JDK 绝对路径
    - 更新 Java 插件执行命令,使用绝对路径
    - 移除 RPM 构建中对 SQLE_JAVA_HOME 环境变量的设置
    @actiontech-bot actiontech-bot requested a review from iwanghc March 26, 2025 07:02
    Copy link

    github-actions bot commented Mar 26, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 7706abe)

    🎫 Ticket compliance analysis ❌

    2307 - Not compliant

    Non-compliant requirements:

    • 未解决decimal值最大化的相关问题

    Requires further human verification:

    • 需确认此PR是否与最大化decimal值的需求相关
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    错误处理

    getJdkPath 函数中,如果无法获取当前目录,函数将返回空字符串。然而,调用该函数的地方未对返回值进行处理,可能导致 javaPluginCmd 使用空路径,从而引发执行错误。

    func getJdkPath() string {
    	nowDir, err := os.Getwd()
    	if err != nil {
    		log.NewEntry().Errorf("failed to get directory: %v", err)
    		return ""
    	}
    	jdkPath := filepath.Join(nowDir, "jdk")
    	return jdkPath
    }

    Copy link

    github-actions bot commented Mar 26, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 7706abe
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    验证 JDK 路径

    在使用 jdkPath 之前,检查其是否为空,并在为空时进行适当的错误处理,以防止后续命令格式错误。

    sqle/driver/plugin_manager.go [195]

     jdkPath := getJdkPath()
    +if jdkPath == "" {
    +    log.NewEntry().Error("获取 JDK 路径失败。")
    +    return fmt.Errorf("invalid JDK path")
    +}
    Suggestion importance[1-10]: 7

    __

    Why: 添加对 jdkPath 是否为空的检查,可以确保在获取 JDK 路径失败时适当地处理错误,防止后续命令格式错误。

    Medium
    改进错误处理

    getJdkPath 返回错误,而不是空字符串,以便调用者可以适当地处理错误情况。

    sqle/driver/plugin_manager.go [245-253]

    -func getJdkPath() string {
    +func getJdkPath() (string, error) {
     	nowDir, err := os.Getwd()
     	if err != nil {
     		log.NewEntry().Errorf("failed to get directory: %v", err)
    -		return ""
    +		return "", err
     	}
     	jdkPath := filepath.Join(nowDir, "jdk")
    -	return jdkPath
    +	return jdkPath, nil
     }
    Suggestion importance[1-10]: 6

    __

    Why: 让 getJdkPath 返回错误可以增强错误处理,使调用者能够更好地处理获取 JDK 路径失败的情况,但需要相应地更新所有调用者以处理新的错误返回。

    Low

    Previous suggestions

    Suggestions up to commit 6e3b108
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    添加错误处理

    应处理 os.Getwd() 返回的错误,以避免在无法获取当前工作目录时导致 jdkPath 错误。

    sqle/driver/plugin_manager.go [245-249]

     func getJdkPath() string {
    -    nowDir, _ := os.Getwd()
    -    jdkPath := nowDir + "/jdk"
    +    nowDir, err := os.Getwd()
    +    if err != nil {
    +        log.NewEntry().Errorf("获取当前工作目录失败: %v", err)
    +        return ""
    +    }
    +    jdkPath := filepath.Join(nowDir, "jdk")
         return jdkPath
     }
    Suggestion importance[1-10]: 7

    __

    Why: Adding error handling for os.Getwd() prevents potential runtime errors when the current working directory cannot be retrieved.

    Medium
    验证 jdkPath 的有效性

    确认 jdkPath 指向的 JDK 路径存在且可执行,以防止在执行 Java 插件时出现运行时错误。

    sqle/driver/plugin_manager.go [210]

    +if _, err := os.Stat(filepath.Join(jdkPath, "bin", "java")); os.IsNotExist(err) {
    +    log.NewEntry().Errorf("JDK 路径无效或 Java 可执行文件不存在: %s", jdkPath)
    +    return err
    +}
     javaPluginCmd := fmt.Sprintf("%s/bin/java -jar %s", jdkPath, cmdBase)
    Suggestion importance[1-10]: 7

    __

    Why: Verifying the existence of the JDK path enhances runtime safety by preventing execution failures when the Java executable is missing.

    Medium

    - 使用 filepath.Join 代替字符串拼接,以确保跨平台兼容性
    - 添加错误处理,防止在获取当前目录时出现未捕获的错误
    Copy link

    Persistent review updated to latest commit 7706abe

    @iwanghc iwanghc merged commit 6000de8 into main Mar 27, 2025
    4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants