[bug](jdbc) test_jdbc_connection rpc should check enable_java_support firstly#62486
Open
zhangstar333 wants to merge 1 commit intoapache:masterfrom
Open
[bug](jdbc) test_jdbc_connection rpc should check enable_java_support firstly#62486zhangstar333 wants to merge 1 commit intoapache:masterfrom
zhangstar333 wants to merge 1 commit intoapache:masterfrom
Conversation
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
No blocking issues found in this change.
Critical checkpoints:
- Goal / correctness: The goal is to prevent
test_jdbc_connectionfrom entering the JNI path whenenable_java_support=false. The new early return inbe/src/service/internal_service.cppdoes that before the request is queued orJniReader::open()can callJni::Env::Get(), so it addresses the reported crash. FE already converts non-OKPStatusfrom this RPC into a user-facing error. - Scope / minimality: The modification is small, focused, and limited to the affected RPC entry point.
- Concurrency: No new concurrency is introduced. The handler now either fails fast on the RPC thread or follows the existing heavy-work-pool path unchanged. No shared-state or lock behavior changes.
- Lifecycle / static initialization: No lifecycle or static-init risk is added. The guard actually reduces exposure to JVM/TLS initialization paths when Java support is disabled.
- Configuration: No new config is added. The change correctly follows the existing
enable_java_supportcontract and restart requirement. - Compatibility: No FE/BE protocol, symbol, storage-format, or rolling-upgrade compatibility issue is introduced.
- Parallel code paths: Existing HDFS/JNI entry points already guard on
enable_java_support; this bringstest_jdbc_connectionin line with those paths. - Conditional check quality: The new condition is justified and straightforward because BE only initializes JNI when Java support is enabled in
doris_main.cpp. - Test coverage: No automated test was added for this crash path. That is the main residual risk, but given the very localized fix and the direct alignment with existing guard patterns, I do not see it as a blocker.
- Test results changed: Not applicable.
- Observability: Existing status propagation is sufficient for this path; no additional logging/metrics appear necessary.
- Transaction / persistence / data-write impact: Not applicable.
- FE-BE variable passing: Not applicable.
- Performance: Slightly improved in the disabled-Java case by failing before queueing, thrift deserialization, and JNI setup.
- Other issues: None found.
Summary opinion: The fix is correct, minimal, and consistent with adjacent JNI/HDFS guard patterns. The only notable gap is missing automated coverage for this RPC failure path.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
if enable_java_support=false, and the rpc of test_jdbc_connection may cause coredump
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)