feat: support fury serializer #356
Conversation
|
Warning Rate limit exceeded@funky-eyes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 40 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes integrate the Apache Fury serialization framework into the project, enhancing serialization capabilities with the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SerializerManager
participant FurySerializer
Client->>SerializerManager: Request Serializer (Fury)
SerializerManager->>FurySerializer: Instantiate FurySerializer
FurySerializer-->>SerializerManager: Return FurySerializer Instance
SerializerManager-->>Client: Return FurySerializer
Client->>FurySerializer: Serialize Object
FurySerializer-->>Client: Return Serialized Data
sequenceDiagram
participant Client
participant FurySerializer
Client->>FurySerializer: Serialize Invalid Object
FurySerializer-->>Client: Throw CodecException
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- pom.xml (2 hunks)
- src/main/java/com/alipay/remoting/serialization/HessianSerializer.java (1 hunks)
- src/main/java/com/alipay/remoting/serialization/SerializerManager.java (2 hunks)
- src/main/java/com/alipay/remoting/serialization/fury/FurySerializer.java (1 hunks)
- src/test/java/com/alipay/remoting/serialization/fury/FurySerializerTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/alipay/remoting/serialization/HessianSerializer.java
Additional comments not posted (15)
src/main/java/com/alipay/remoting/serialization/fury/FurySerializer.java (4)
32-39: LGTM! Class members are well-defined.The static list
REGISTRY_LISTand theThreadSafeFuryinstance are appropriately defined for the class functionality.
41-47: LGTM! Proper error handling inserializemethod.The
serializemethod correctly handles serialization errors by wrapping them in aCodecException.
55-57: LGTM! Simple and effectiveregistrymethod.The
registrymethod correctly adds classes to theREGISTRY_LIST.
34-39: LGTM! Proper initialization in constructor.The constructor correctly initializes the
furyinstance with a custom class loader and registers classes from theREGISTRY_LIST.src/main/java/com/alipay/remoting/serialization/SerializerManager.java (4)
39-39: LGTM! New constantFuryis well-defined.The addition of the new constant
Furyis appropriate for adding support forFurySerializer.
20-20: LGTM! Updatedserializersarray is well-defined.The updated
serializersarray is appropriate for adding support forFurySerializer.
45-56: LGTM! Proper support forFurySerializeringetSerializermethod.The
getSerializermethod correctly implements support forFurySerializerwith proper locking and initialization.
Line range hint
58-69:
LGTM!addSerializermethod is well-implemented.The
addSerializermethod correctly supports dynamic addition of serializers.src/test/java/com/alipay/remoting/serialization/fury/FurySerializerTest.java (5)
38-43: LGTM! Class members are well-defined for testing.The static
serializerinstance and the static block for initialization are appropriate for testing purposes.
45-55: LGTM! Proper concurrency test inconcurrentSerializeTestmethod.The
concurrentSerializeTestmethod correctly tests the thread safety of theFurySerializerwith proper concurrency handling usingCountDownLatch.
57-65: LGTM! Proper error handling test intestSerializeErrormethod.The
testSerializeErrormethod correctly verifies that theFurySerializerthrows aCodecExceptionwith the expected cause.
67-75: LGTM! Successful serialization test intestSerializemethod.The
testSerializemethod correctly verifies that theFurySerializersuccessfully serializes an object.
77-99: LGTM! Proper implementation ofMyThreadclass for concurrency testing.The
MyThreadclass correctly tests the concurrent serialization and deserialization capabilities of theFurySerializer.pom.xml (2)
90-90: LGTM! Adding thefury.versionproperty.The addition of the
fury.versionproperty is consistent with the existing version management strategy.
100-105: LGTM! Adding thefury-coredependency.The addition of the
fury-coredependency with a scope ofprovidedis appropriate and follows the existing pattern for adding dependencies.
| @Override | ||
| public <T> T deserialize(byte[] data, String classOfT) throws CodecException { | ||
| return (T)fury.deserialize(data); | ||
| } |
There was a problem hiding this comment.
Add type safety to deserialize method.
The deserialize method uses unchecked casting which can lead to ClassCastException. Consider adding type safety.
- public <T> T deserialize(byte[] data, String classOfT) throws CodecException {
- return (T)fury.deserialize(data);
+ public <T> T deserialize(byte[] data, Class<T> classOfT) throws CodecException {
+ return classOfT.cast(fury.deserialize(data));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.
| @Override | |
| public <T> T deserialize(byte[] data, String classOfT) throws CodecException { | |
| return (T)fury.deserialize(data); | |
| } | |
| @Override | |
| public <T> T deserialize(byte[] data, Class<T> classOfT) throws CodecException { | |
| return classOfT.cast(fury.deserialize(data)); |
There was a problem hiding this comment.
这个问题我无法决定是否如此修改,因为会涉及改动上层序列化接口
There was a problem hiding this comment.
@funky-eyes, I understand that changing the deserialize method's signature might involve modifications to the upper-level serialization interface and other dependent classes. The method is used in several places across the codebase, including FurySerializerTest.java, HessianSerializerTest.java, RpcResponseCommand.java, and more.
Given the widespread usage, this change would require careful consideration and testing. It might be best to discuss this further or address it in a separate PR to ensure all dependencies are handled properly. Let me know how you would like to proceed.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all invocations of the `deserialize` method in the codebase to understand its usage and potential impact of changing its signature.
rg --context 5 'deserialize\('
Length of output: 18313
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pom.xml (2 hunks)
- src/main/java/com/alipay/remoting/serialization/SerializerManager.java (2 hunks)
- src/test/java/com/alipay/remoting/serialization/fury/FurySerializerTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- pom.xml
- src/main/java/com/alipay/remoting/serialization/SerializerManager.java
- src/test/java/com/alipay/remoting/serialization/fury/FurySerializerTest.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/maven.yml (1 hunks)
- src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java (2 hunks)
Additional comments not posted (4)
.github/workflows/maven.yml (1)
35-35: LGTM! Verify compatibility with Java 17.The addition of Java 17 to the Java version matrix is a good step to ensure compatibility with newer Java features. Ensure that the codebase is compatible with Java 17.
src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java (3)
69-69: LGTM! Improved assertion clarity.The change from
Assert.assertTrue(remotingContextList.size() == 2)toAssert.assertEquals(2, remotingContextList.size())improves the clarity and consistency of the assertion.
92-92: LGTM! Improved assertion specificity.The change from
Assert.assertTrue(remotingCtx != remotingContext)toAssert.assertNotSame(remotingCtx, remotingContext)more accurately conveys the intent of checking that the two references are not the same object in memory.
68-68: Verify the impact of removing the timeout parameter.The removal of the timeout parameter from
countDownLatch.await()may affect how long the test waits for the latch to count down. Ensure that this change does not cause the test to hang indefinitely.Verification successful
The removal of the timeout parameter from
countDownLatch.await()does not cause the test to hang indefinitely.
- The
countDownLatch.countDown()method is called within the test file, ensuring the latch is decremented and the test proceeds.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the timeout parameter from `countDownLatch.await()`. # Test: Run the test suite. Expect: No hanging tests. ./mvnw testLength of output: 133
Script:
#!/bin/bash # Search for all instances of `countDownLatch.await()` in the test file to analyze their usage. rg 'countDownLatch.await' src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java -A 5Length of output: 377
Script:
#!/bin/bash # Search for all instances of `countDownLatch.countDown()` in the test file to ensure the latch is properly decremented. rg 'countDownLatch.countDown' src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java -A 5Length of output: 296
|
Great, I'll spend some time to check this PR. |
| return new ByteArrayOutputStream(); | ||
| } | ||
| }; | ||
| private static ThreadLocal<ByteArrayOutputStream> localOutputByteArray = ThreadLocal.withInitial( |
There was a problem hiding this comment.
bolt用于多个基础组件，最好不要用高版本的用法保持线下兼容
| //public static final byte Json = 2; | ||
|
|
||
| public static final byte Fury = 3; |
There was a problem hiding this comment.
另，bolt作为底层组件需要考虑引入依赖版本的兼容性，如现在rpc里使用fury版本是0.4，如果和当前0.6版本不兼容就会有问题
There was a problem hiding this comment.
另，bolt作为底层组件需要考虑引入依赖版本的兼容性，如现在rpc里使用fury版本是0.4，如果和当前0.6版本不兼容就会有问题
这个不太好改，因为fury捐给apache后，对老的包全部做了迁移，没有做向下兼容，导致0.6跟0.4不兼容，0.4跟0.6也不兼容。
当然序列化后的数据是兼容的，但是包和类都不同了，可以说0.4和0.6压根就不是一个依赖，相当于引入了2个依赖，实际上应该不会有冲突，因为各自用的各自的类，在序列化反序列化方面是兼容的即可。
There was a problem hiding this comment.
是否考虑rpc先把版本提升上来，统一维护一个版本就好了
There was a problem hiding this comment.
是否考虑rpc先把版本提升上来，统一维护一个版本就好了
好的,我去看看rpc那块的实现,考虑先提交到sofa-rpc侧.
There was a problem hiding this comment.
个人认为apache fury已经进入apache孵化器,未来的迭代,稳定性也会更高,性能根据其测试报告非常优秀,并且目前sofa-bolt自带的序列化器只有hessian,应该扩充一些序列化器避免用户自行实现.
目前pr中遗留2个问题需要跟社区讨论下:
Summary by CodeRabbit
New Features
FurySerializer, enhancing serialization capabilities within the application.Bug Fixes
Tests
FurySerializer, validating successful and concurrent serialization scenarios.Chores