V5.13.3 release to v5.14.0 #1486
Conversation
…type issue (sofastack#1481) Co-authored-by: liujianjun.ljj <[email protected]>
Co-authored-by: liujianjun.ljj <[email protected]>
…ck#1480) Co-authored-by: liujianjun.ljj <[email protected]>
…ofastack#1482) (cherry picked from commit d20062f) Co-authored-by: 辰霖 <[email protected]>
…oo many ping problem (sofastack#1483) Co-authored-by: liujianjun.ljj <[email protected]>
…s scene (sofastack#1484) Co-authored-by: liujianjun.ljj <[email protected]>
Co-authored-by: liujianjun.ljj <[email protected]>
…elease_to_v5.14.0 # Conflicts: # all/pom.xml # bom/pom.xml # core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java # pom.xml # remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java
WalkthroughThe pull request includes modifications across multiple modules. In the core API, constant naming conventions and timeout retrieval logic were refined while expanding the asynchronous invocation conditions. In the remoting module, new OpenTracing dependencies were introduced and various interceptors, tracing adapters, and stream handlers were updated to improve context management, error handling, and class loader usage. Additionally, test cases were added to validate timeout behavior, service inheritance, and streaming requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientHeaderInterceptor
participant TracerAdapter
participant ServerInterceptor
participant Server
Client->>ClientHeaderInterceptor: Send RPC request
ClientHeaderInterceptor->>TracerAdapter: Call beforeSend (with method details)
TracerAdapter-->>ClientHeaderInterceptor: Return tracing context
ClientHeaderInterceptor->>ServerInterceptor: Forward request with metadata
ServerInterceptor->>Server: Process request\n(begin stream)
Server-->>ServerInterceptor: Stream response(s)
ServerInterceptor-->>ClientHeaderInterceptor: Forward responses with error handling
ClientHeaderInterceptor-->>Client: Deliver responses
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/ParentService.java (1)
25-31: Consider adding JavaDoc for methods:
Although the interface is straightforward, adding JavaDoc forsayHello()andparentSayHelloServerStream()would clarify their parameters, expected behavior, and potential usage scenarios—especially for streaming methods where additional detail is often helpful.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/message/triple/stream/ClientStreamObserverAdapter.java (1)
34-34: Restrict logger visibility:
It’s common practice to declare SLF4J loggers asprivate static final. Making it private aligns with typical conventions and avoids exposing the logger unnecessarily.- public static final Logger LOGGER = LoggerFactory.getLogger(ClientStreamObserverAdapter.class); + private static final Logger LOGGER = LoggerFactory.getLogger(ClientStreamObserverAdapter.class);remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapterTest.java (1)
49-49: Updated test to match new method signature.The test has been updated to call the new version of
beforeSendthat takes an additionalMethodDescriptorparameter (passing null here). This keeps the test compatible with the updated API.Consider enhancing this test to also verify behavior with a non-null
MethodDescriptorparameter to ensure complete coverage of the new functionality.-TripleTracerAdapter.beforeSend(sofaRequest, consumerConfig, metadata, null); +// Add test with a mock MethodDescriptor +MethodDescriptor<Object, Object> mockMethod = MethodDescriptor + .newBuilder() + .setType(MethodDescriptor.MethodType.UNARY) + .setFullMethodName("testService/testMethod") + .setRequestMarshaller(null) + .setResponseMarshaller(null) + .build(); + +// Test with the mock method descriptor +Metadata metadataWithMethod = new Metadata(); +TripleTracerAdapter.beforeSend(sofaRequest, consumerConfig, metadataWithMethod, mockMethod); +// Add assertions to verify behavior with the method descriptortest/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java (1)
76-76: Consider removing or replacing long sleeps in test code.Using
Thread.sleep(5000);can slow down the test suite. Prefer more deterministic synchronization methods, like waiting for a specific server start signal or using more granular countdown latches, if possible.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java (1)
72-124: ForwardingServerCall override is comprehensive.
- Each overridden method (
sendHeaders,sendMessage,close) includes try-catch blocks to capture exceptions—a good practice for robust error handling.- The final block in
closecorrectly callsTripleTracerAdapter.serverSend(...)on non-OK status.Consider factoring out repeated error-logging logic to improve maintainability, but otherwise fine.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java (3)
70-70: Consider null checks for consumerConfig and sofaRequest before calling TripleTracerAdapter.
In scenarios where the context might be incomplete, a null ConsumerConfig or SofaRequest could trigger unexpected behavior.Also applies to: 74-78
114-135: Check for potential concurrency or resource leaks in onClose for async requests.
ClearingsofaTraceContextand removing invocation contexts in thefinallyblock is good practice. Confirm no other asynchronous threads rely on these contexts while the event is processed.
148-156: Ensure robust error handling logic in sendMessage.
Catching and logging errors is helpful. In certain streaming scenarios, consider whether additional fallback or partial cleanup might be required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java(1 hunks)core/api/src/main/java/com/alipay/sofa/rpc/config/ConsumerConfig.java(1 hunks)core/api/src/main/java/com/alipay/sofa/rpc/core/request/SofaRequest.java(1 hunks)core/api/src/test/java/com/alipay/sofa/rpc/config/ConsumerConfigTest.java(1 hunks)remoting/pom.xml(1 hunks)remoting/remoting-triple/pom.xml(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java(4 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/message/triple/stream/ClientStreamObserverAdapter.java(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/GenericServiceImpl.java(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java(2 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java(4 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientInvoker.java(2 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/TripleExceptionUtils.java(2 hunks)remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapterTest.java(1 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/HelloService.java(1 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/HelloServiceImpl.java(1 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/ParentService.java(1 hunks)test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: sca
🔇 Additional comments (39)
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/ParentService.java (1)
1-16: License header review:
These lines look like a standard ASF license header. No issues found here.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/message/triple/stream/ClientStreamObserverAdapter.java (6)
26-27: Logger imports:
Importing SLF4J Logger and LoggerFactory is the standard approach for logging.
42-43: New field for class loader:
Adding the finalClassLoaderfield appears proper for handling dynamic loading. No immediate issues identified.
44-55: Constructor refactoring with class loader:
The new constructor parameterclassLoaderis a clean way to pass in the relevant loader context. Consider adding a quick null check or fallback for safety ifclassLoadermight be null.Please confirm whether
classLoadercan be null in the calling code. If so, you may want to handle it gracefully or document that it must be non-null.
65-79: Thread context class loader usage in onNext:
Switching the thread context class loader around each invocation can be fragile if multiple threads use the same observer instance concurrently. Confirm that eachClientStreamObserverAdapteris thread-confined or concurrency-safe.Would you like me to generate a script to check if instances of
ClientStreamObserverAdaptermight be shared among multiple threads?
84-90: Same concurrency concern in onError:
The pattern of setting and restoring the context class loader is repeated here. The same concurrency considerations apply.
95-101: Same concurrency concern in onCompleted:
Again, setting and restoring the thread context class loader is ideal for single-thread usage. Please ensure this is not shared across multiple threads.remoting/remoting-triple/pom.xml (1)
69-72:❓ Verification inconclusive
Add tracer-opentracing dependency:
Ensure that this dependency version is managed properly. Having an explicit version in<dependencyManagement>or confirming its inherited version can prevent mismatched library issues.
🏁 Script executed:
#!/bin/bash # Description: Search for the dependencyManagement entry for sofa-rpc-tracer-opentracing in the repo. fd pom.xml --exec rg -A 5 'dependencyManagement' | rg -A 10 'sofa-rpc-tracer-opentracing'Length of output: 87
Attention: Verify Dependency Version Management for Tracer-Opentracing
Our automated search did not locate any
<dependencyManagement>entry for thesofa-rpc-tracer-opentracingdependency. Please manually verify that its version is inherited from a parent POM or, if not, add an explicit version in the<dependencyManagement>section to avoid potential mismatches.
- Confirm that a parent POM is managing the version for this dependency.
- If not managed, update the dependency management to include the appropriate version.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/HelloService.java (1)
21-21: Interface now extends ParentServiceThe
HelloServiceinterface has been updated to extendParentService, which means it now inherits methods and contracts from the parent interface. This change enhances the service capability by inheriting the parent's methods (sayHelloandparentSayHelloServerStreamas mentioned in the summary).Ensure that all implementations of
HelloServicenow also implement the methods required byParentService.remoting/pom.xml (1)
91-95: Added OpenTracing dependencyA new dependency for
sofa-rpc-tracer-opentracinghas been added to the dependency management section. This addition enables OpenTracing functionality for distributed request tracing across the RPC infrastructure.The version is correctly tied to the parent project version (
${project.parent.version}), ensuring consistent versioning across modules.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/TripleServer.java (1)
146-147: Improved keep-alive configuration for gRPC connectionsTwo important keep-alive configurations have been added to the
NettyServerBuilder:
.permitKeepAliveTime(1, TimeUnit.SECONDS)- Sets the minimum time between client keep-alive pings.permitKeepAliveWithoutCalls(true)- Allows clients to send keep-alive pings even when there are no active callsThese changes enhance connection stability, especially in environments with proxies or firewalls that might close idle connections, reducing connection reset issues and improving overall reliability.
core/api/src/main/java/com/alipay/sofa/rpc/core/request/SofaRequest.java (1)
308-314: Enhanced isAsync detection for streaming operationsThe
isAsync()method has been improved to recognize more invocation types as asynchronous. In addition to the traditional callback and future patterns, it now correctly identifies all streaming types (bi-directional, server-side, and client-side streaming) as asynchronous operations.This change aligns with the true nature of streaming operations, which are inherently asynchronous, and ensures they will be properly handled by asynchronous processing mechanisms within the framework.
core/api/src/test/java/com/alipay/sofa/rpc/config/ConsumerConfigTest.java (1)
1-56: Test for method timeout retrieval added - good coverage!This new test class provides good coverage for the
getMethodTimeoutfunctionality of theConsumerConfigclass. The test verifies:
- Default timeout retrieval when no method config exists
- Default timeout propagation when method config exists without timeout
- Custom timeout retrieval when explicitly set
- Negative timeout handling
The test is well-structured and comprehensive.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/TripleExceptionUtils.java (2)
19-23: New exception imports added to support mapping from gRPC Status to Throwable types.These imports are necessary for the new method implementation.
36-46: Effective mapping from gRPC Status to Throwable types.This new method provides a clean mapping between gRPC Status codes and SOFA RPC exception types, which allows for consistent error handling throughout the codebase. The mapping correctly handles:
- OK status → null (no exception)
- UNAVAILABLE status → SofaRouteException
- DEADLINE_EXCEEDED status → SofaTimeOutException
- Other statuses → generic SofaRpcException with UNKNOWN type
The method preserves the original status description and cause for better error reporting.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java (1)
99-99:✅ Verification successful
Changed to include inherited methods in stream call type caching.
Replacing
getDeclaredMethods()withgetMethods()is a significant improvement that ensures all methods, including those inherited from parent classes, are properly considered when caching stream call types. This change helps maintain consistency in RPC invocation behavior across class hierarchies.
🏁 Script executed:
#!/bin/bash # Check for classes that extend interfaces with stream methods to verify the impact of this change # Find Java interfaces that might have streaming methods echo "Looking for interfaces with potential stream methods..." rg -l "extends.*implements" --type java | xargs grep -l "SofaStreamObserver" || echo "No stream interfaces found with inheritance"Length of output: 294
Verification: Inherited Methods Caching Update
The change from using
getDeclaredMethods()togetMethods()correctly ensures that inherited methods are now included in stream call type caching. Our verification script did not reveal any interfaces (e.g., those involvingSofaStreamObserver) with inheritance issues, confirming that the updated approach does not introduce unintended side effects.
- Location:
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java(Line 99)- Change Impact: Now considers all methods (inherited and declared), which improves consistency in RPC invocation behavior across class hierarchies.
core/api/src/main/java/com/alipay/sofa/rpc/config/ConsumerConfig.java (1)
936-940: Improved timeout handling logic with fallback mechanismThe method now explicitly checks for null or zero timeout values and returns the default timeout in those cases. This is a clearer implementation that makes the fallback behavior more explicit.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/server/triple/GenericServiceImpl.java (3)
134-142: Enhanced classloader management in onNext methodThe addition of proper class loader handling with try/finally blocks ensures that the correct service classloader is used during stream processing and then restored, preventing potential classloader leaks or context issues.
147-152: Enhanced classloader management in onError methodAdding the try/finally block to properly manage the thread's context classloader during error handling ensures consistent classloader management across all callback methods.
158-163: Enhanced classloader management in onCompleted methodSimilar to the other methods, proper classloader management has been added to ensure the service classloader is used during stream completion and then restored afterward.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/HelloServiceImpl.java (2)
81-84: Implementation of sayHello method for interface inheritanceThis adds the unary request/response method implementation needed to satisfy the
ParentServiceinterface contract.
86-90: Implementation of streaming method from parent interfaceCorrectly delegates to the existing implementation to avoid code duplication while satisfying the interface contract from the parent service.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientInvoker.java (2)
196-198: Added classloader parameter to stream adapterThis change passes the current classloader to the ClientStreamObserverAdapter, enabling proper classloader handling during bi-directional streaming calls.
235-235: Added classloader parameter to server stream adapterSimilarly to the bi-directional streaming case, now passing the current classloader to the ClientStreamObserverAdapter for server streaming calls to ensure proper class loading during RPC operations.
test/test-integration/src/test/java/com/alipay/sofa/rpc/test/triple/stream/TripleGenericStreamTest.java (1)
88-119: Test logic validation and coverage are good.
- The newly introduced method
testTripleParentCall()correctly verifies streaming responses and completion signals.- The
CountDownLatchusage appears correct; ensuring 5 onNext calls plus one onCompleted equals 6 latch counts.- The final asserts checking list size and
receivedFinishstrongly validate the desired behavior.No issues found that block merging.
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcConstants.java (1)
131-139: Naming convention update aligns with underscore style.Renaming the constants to
client_stream,server_stream, andbi_streamis consistent. No functional concerns are observed.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java (7)
59-61: Logical flow initialization.Acquiring the
ServerServiceDefinitionbefore constructingSofaRequestandSofaResponseis clear and well-structured. No concerns at this point.
62-64: Object creation is straightforward.Creating
SofaRequest/SofaResponseinstances here is consistent. Ensure that any relevant fields (like method or interface metadata) are properly set in upstream logic.
65-65: Context injection.
ctxWithSpaneffectively injects tracing context into the gRPC call. No concerns.
67-69: Debug logging.Logging request headers in debug mode is useful for diagnostics. No issues here.
125-132: Intercept call logic with context.
- Properly handles any exceptions during the intercept phase by logging and rethrowing a
StatusRuntimeException.- Good approach for bridging gRPC and the tracing logic.
No further issues.
133-213: ForwardingServerCallListener overrides manage concurrency and tracing.
onCancel,onComplete,onMessage, andonHalfCloseeach set the trace and context, handle operations, and then finalize withTripleTracerAdapter.serverSend.- Removing the
RpcInvokeContextand clearing theSofaTraceContextin thefinallyblock ensures no context leaks into subsequent calls.Overall, a robust handling of server streaming lifecycle events.
214-216: Final cleanup.Clearing the
RpcInvokeContextandSofaTraceContextafter intercept completion avoids leftover context data. This is consistent with best practices for ephemeral tracing data.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ClientHeaderClientInterceptor.java (2)
19-21: New imports appear valid and necessary for tracing enhancements.
No issues found; these additional imports align with the updated tracing and event handling logic.Also applies to: 23-23, 27-29, 32-32
94-109: Confirm thread safety of the async context manipulation in onMessage.
We see that the async request logic pushes/pops the tracer context. This pattern is correct but ensure that exceptions thrown bysuper.onMessagewon’t leave the context in a partially updated state.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java (4)
44-45: New imports for tracer tags and gRPC context.
These additions correctly support the new methods for enriched tracing functionality.Also applies to: 48-48
82-85: Deprecation of the old beforeSend method.
Marking it as deprecated while directing usage to the new overloaded version is appropriate for backward compatibility.
87-145: Enhanced beforeSend with MethodDescriptor support.
The logic for setting invokeType and tagging spans according to method type is well-structured. Ensure all streaming scenarios are covered, and confirm that synchronous calls default to “sync” gracefully.
326-345: Refactoring serverSend with an additional Context parameter.
The updated implementation is coherent; pushing the original span from the provided context if the current span is null is a valuable fallback. Maintain the deprecated method to preserve existing integrations without forcing an immediate upgrade.
v5.13.3 merge into main branch.
Summary by CodeRabbit
New Features
Refactor
Tests