fix: add logging to exception handling in ProtoMessageUtil - #15628
fix: add logging to exception handling in ProtoMessageUtil#15628lxcxjxhx wants to merge 4 commits into
Conversation
- Add SLF4J logger to capture previously silent exceptions - Replace empty catch blocks with debug logging for better troubleshooting - Maintain backward compatibility with legacy protocol formats Local environment limitations: relying on CI/CD automated testing for full validation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
KomachiSion
left a comment
KomachiSion
left a comment
There was a problem hiding this comment.
One more thing:
Maybe we can remove the old GetRequest and Log, which is for upgrading from becore 2.x. And now 3.x is not support upgraded from 1.x.
| LOGGER.debug("Failed to parse legacy GetRequest, will try Log format", e); | ||
| } | ||
|
|
There was a problem hiding this comment.
indent changed might cause spotless check can't pass
Remove deprecated GetRequest and Log compatibility code since Nacos 3.x does not support upgrading from 1.x. This simplifies the parse method and removes unused conversion methods.
|
Updated based on @KomachiSion's review feedback:
Since Nacos 3.x doesn't support upgrading from 1.x, the legacy format fallback is no longer needed. |
- Remove tests for deleted convertToReadRequest/convertToWriteRequest methods - Update testParseReadRequest/testParseWriteRequest to verify legacy fallback now throws ConsistencyException as expected - Fix testParseWithCorruptRequestTypeField to expect exception
|
Also updated the test file:
All tests should pass with the simplified parse method. |
|
Mockito's inline mock maker supports static mocks based on the Instrumentation API. ci not pass, it seems Mockito's inline can't be remove. |
nacos-community
left a comment
nacos-community
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @lxcxjxhx — logging in this parser is genuinely useful. However, the PR as it stands goes well beyond "add logging" and introduces two breaking changes that need to be separated out before this can be merged.
Summary
The PR title/description describe a minimal logging addition, but the diff (a) removes the legacy GetRequest/Log compatibility fallbacks in ProtoMessageUtil.parse, and (b) deletes the mockito-inline test dependency from the root and ai POMs. Both are independent, higher-risk changes that are not disclosed in the description.
Findings
- [Critical]
ProtoMessageUtil.java— removing the legacy fallbacks breaks JRaft log replay for old-format entries (rolling upgrade / mixed-version risk).parseis invoked byNacosStateMachine.onApplyon persisted raft data. - [Critical]
pom.xml/ai/pom.xml— removingmockito-inlineon Mockito 4.11.0 breaks static mocking; 22 test files inai/src/testusemockStatic. - [Info]
ProtoMessageUtil.java:60— the log only fires on the tagged-path failure; consider logging on the final failure path too.
Suggestions
Please split this PR:
- Keep a focused PR that only adds logging (retain the existing fallbacks and both pom entries). That part is welcome.
- If removing the legacy
GetRequest/Logcompatibility is actually intended, open a dedicated issue/spec change for maintainer discussion first (per the repo's spec-first convention) rather than bundling it here.
Also update the description to reflect the real scope. Happy to re-review once narrowed to the logging change.
Automated review by github-manager-bot
| * @param bytes An array of bytes | ||
| * @return Message | ||
| */ | ||
| public static Message parse(byte[] bytes) { |
There was a problem hiding this comment.
[Critical] This PR removes the legacy compatibility fallbacks (GetRequest -> ReadRequest and Log -> WriteRequest conversion), which is far beyond the stated scope of "add logging".
ProtoMessageUtil.parse is called from NacosStateMachine.onApply when replaying persisted JRaft log entries. Raft logs written by older Nacos versions may contain old-format GetRequest/Log entries; after this change they would all fail with ConsistencyException during replay, which can break rolling upgrades and mixed-version clusters. The original code intentionally kept these paths ("Internally, the protobuf new and old objects are compatible").
If dropping this compatibility is really intended, it needs a dedicated issue/spec discussion and maintainer agreement (per the repo's spec-first convention), not a silent removal inside a logging PR. Please restore the fallbacks in this PR.
| return convertToWriteRequest(log); | ||
| } catch (Throwable ignore) { | ||
| } catch (Throwable e) { | ||
| LOGGER.debug("Failed to parse protocol request", e); |
There was a problem hiding this comment.
[Info] Adding the log statement itself is fine, but note it only fires on the tagged-path failure. If the fallbacks are restored, consider keeping the log at the final failure point so unparseable payloads are still observable at a useful level (debug may be too low to notice in production; warn on the final ConsistencyException path is worth considering).
| @@ -84,11 +84,6 @@ | |||
| <artifactId>mockito-core</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
There was a problem hiding this comment.
[Critical] Removing mockito-inline is unrelated to the stated purpose of this PR and breaks the ai module test suite: the project uses Mockito 4.11.0, where Mockito.mockStatic requires the inline mock maker, and 22 test files under ai/src/test use mockStatic/MockedStatic (e.g. AiPipelineModuleConfigTest). Without this dependency those tests fail at runtime. Please revert the pom changes.
| @@ -652,11 +652,6 @@ | |||
| <artifactId>mockito-core</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
There was a problem hiding this comment.
[Critical] Same concern as in ai/pom.xml: removing mockito-inline from the root dependencyManagement while the project is on Mockito 4.11.0 breaks static mocking used across module tests. Please revert.
nacos-community
left a comment
nacos-community
left a comment
There was a problem hiding this comment.
Summary
Thanks for the new commits, @lxcxjxhx. The logging addition itself is sound (slf4j class logger, DEBUG level, throwable passed, no payload leaked), but the PR still bundles two breaking changes that the previous review asked to separate out, so this remains REQUEST_CHANGES.
Findings
- [Critical]
consistency/src/main/java/com/alibaba/nacos/consistency/ProtoMessageUtil.java:50— This still removes the legacy GetRequest/Log fallbacks from parse() — the opposite of what the previous review asked (retain them here; move removal to a separa... - [Critical]
pom.xml:654— Removing mockito-inline from the inherited test dependencies breaks static mocking across all modules. mockito.version is 4.11.0 (pom.xml:148), where Mockito.mo... - [Critical]
ai/pom.xml:86— Same mockito-inline removal: 34 test files under ai/src/test alone use Mockito.mockStatic (e.g. ai/src/test/java/com/alibaba/nacos/ai/service/a2a/migration/A2aM... - [Warning]
consistency/src/main/java/com/alibaba/nacos/consistency/ProtoMessageUtil.java:60— The debug log only fires when parseFrom throws inside the tagged path. Bytes that do not start with REQUEST_TYPE_FIELD_TAG — which after this change includes ev... - [Warning]
consistency/src/test/java/com/alibaba/nacos/consistency/ProtoMessageUtilTest.java:104— These new tests codify the regression as expected behavior ('should throw ConsistencyException since legacy fallback is removed'). Once the fallbacks are restor... - [Info]
consistency/src/main/java/com/alibaba/nacos/consistency/ProtoMessageUtil.java:46— The deleted javadoc line 'Internally, the protobuf new and old objects are compatible' documented a forward-compatibility contract, and the deleted code comment...
Suggestions
To move this forward, please reduce the PR to the pure logging change:
- Restore the legacy
GetRequest/Logfallbacks andconvertToReadRequest/convertToWriteRequestinProtoMessageUtil.parse(they are still live on the JRaft protocol surface viaJRaftUtils.initRpcServerandNacosRuntimeHints; removing them breaks raft log replay for entries written by older versions — seeNacosStateMachine.onApply). - Restore
mockito-inlinein the rootpom.xmlandai/pom.xml(Mockito 4.11.0 needs it forMockito.mockStatic, used by 191 test files). - Also log on the final failure path (bytes without the type-tag prefix currently throw with no log).
- Revert the tests that codify the removed behavior; keep the genuine test improvements (
assertEquals(actual.getClass(), ...),fail()in the corrupt-request test). - If removing the legacy format is genuinely intended, propose it in a dedicated issue/PR following the repo's spec-first convention.
Happy to re-review once rescoped — feel free to @mention me.
Automated review by github-manager-bot
| * @param bytes An array of bytes | ||
| * @return Message | ||
| */ | ||
| public static Message parse(byte[] bytes) { |
There was a problem hiding this comment.
This still removes the legacy GetRequest/Log fallbacks from parse() — the opposite of what the previous review asked (retain them here; move removal to a separate spec-first proposal). parse() has exactly one production caller: NacosStateMachine.onApply (core/src/main/java/com/alibaba/nacos/core/distributed/raft/NacosStateMachine.java:109), which parses persisted JRaft log entries on the follower/replay path. An old-format entry now throws ConsistencyException, which escalates to setErrorAndRollback/ESTATEMACHINE — so a node replaying raft data written by an older Nacos version (rolling upgrade, restart, follower catch-up) fails to apply its log. JRaftUtils.initRpcServer (core/.../JRaftUtils.java:67-80) still registers Log/GetRequest serializers on the raft RPC layer and NacosRuntimeHints still registers these entity classes, so the old format remains part of the live runtime contract. Please restore the fallbacks and the convertToReadRequest/convertToWriteRequest methods; if removal is genuinely intended, open a dedicated issue per the compatibility-deprecation spec and repo spec-first convention.
| @@ -652,11 +652,6 @@ | |||
| <artifactId>mockito-core</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
There was a problem hiding this comment.
Removing mockito-inline from the inherited test dependencies breaks static mocking across all modules. mockito.version is 4.11.0 (pom.xml:148), where Mockito.mockStatic requires the inline mock maker that only mockito-inline activates (no mockito-extensions/org.mockito.plugins.MockMaker override exists in the repo). 191 test files repo-wide call Mockito.mockStatic (config, naming, core, console, client, ai, ...), and no module declares mockito-inline on its own — they all inherit it from this block. Expect mass 'SubclassByteBuddyMockMaker does not support the creation of static mocks' failures. Please restore this entry.
| @@ -84,11 +84,6 @@ | |||
| <artifactId>mockito-core</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
There was a problem hiding this comment.
Same mockito-inline removal: 34 test files under ai/src/test alone use Mockito.mockStatic (e.g. ai/src/test/java/com/alibaba/nacos/ai/service/a2a/migration/A2aMigrationStateServiceTest.java:172) and will fail on Mockito 4.11.0 without the inline mock maker. Restore this entry together with the root pom one.
| return convertToWriteRequest(log); | ||
| } catch (Throwable ignore) { | ||
| } catch (Throwable e) { | ||
| LOGGER.debug("Failed to parse protocol request", e); |
There was a problem hiding this comment.
The debug log only fires when parseFrom throws inside the tagged path. Bytes that do not start with REQUEST_TYPE_FIELD_TAG — which after this change includes every legacy-format entry — skip the try body entirely and reach the ConsistencyException throw below with no log at all, so the main failure mode stays silent (this was also flagged in the previous review and remains unaddressed). Please log on the final failure path, just before the throw. The logger mechanics themselves are fine: slf4j class-level logger, DEBUG level appropriate here, throwable passed correctly, no payload bytes logged.
| @Test | ||
| void testParseReadRequest() { | ||
| void testParseRawReadRequestThrowsException() { | ||
| // Raw ReadRequest bytes without REQUEST_TYPE_FIELD_TAG prefix |
There was a problem hiding this comment.
These new tests codify the regression as expected behavior ('should throw ConsistencyException since legacy fallback is removed'). Once the fallbacks are restored, revert testParseRawReadRequestThrowsException/testParseRawWriteRequestThrowsException to the original raw ReadRequest/WriteRequest parse assertions and restore testParseReadRequest/testParseWriteRequest/testProto/testConvertToReadRequest/testConvertToWriteRequest. The assertEquals(actual.getClass(), ...) fixes and the added fail() assertion in testParseWithCorruptRequestTypeField are genuine improvements worth keeping.
| /** | ||
| * Converts the byte array to a specific Protobuf object. | ||
| * Internally, the protobuf new and old objects are compatible. | ||
| * |
There was a problem hiding this comment.
The deleted javadoc line 'Internally, the protobuf new and old objects are compatible' documented a forward-compatibility contract, and the deleted code comment said the old entities 'will be @deprecated in future' — i.e., a deprecation-first lifecycle, not immediate removal. Also note commit 3edae16's message ('remove legacy GetRequest and Log parsing as per reviewer feedback') inverts what the review actually requested, and the PR description still says this is only a logging change. Please re-scope the PR to the pure logging change and update the description.
3 participants
Motivation
Exception handling in ProtoMessageUtil lacked proper logging, making it difficult to debug parsing issues in production environments.
Changes
ProtoMessageUtil Enhancement
Testing
Checklist