Replace the immutable Collections.emptyList() with a new ArrayList<>() #375
Conversation
WalkthroughReused previously computed Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap as MetaServerBootstrap
participant Handlers as customSessionServerHandlers()
participant Registrar as OpenSessionRegistrar
rect rgb(235,248,255)
Note right of Bootstrap: compute handlers once
Bootstrap->>Handlers: compute handlers (new ArrayList)
Handlers-->>Bootstrap: handlers list
end
rect rgb(235,255,235)
Note right of Bootstrap: reuse handlers when registering
Bootstrap->>Registrar: registerOpenSession( handlers )
Registrar-->>Bootstrap: registration result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (2)
242-246: Avoid double invocation of customSessionServerHandlers().Use the already-fetched variable to prevent recomputation and surprises if subclasses override the method with side effects.
- if (CollectionUtils.isNotEmpty(customSessionServerHandlers)) { - mergedSessionServerHandlers.addAll(this.customSessionServerHandlers()); - } + if (CollectionUtils.isNotEmpty(customSessionServerHandlers)) { + mergedSessionServerHandlers.addAll(customSessionServerHandlers); + }
385-387: Bug: remoteMetaServer not closed in stopServer().
remoteMetaServer.isOpen();is a no-op; the server is never closed, causing a resource leak during shutdown.- if (remoteMetaServer != null && remoteMetaServer.isOpen()) { - remoteMetaServer.isOpen(); - } + if (remoteMetaServer != null && remoteMetaServer.isOpen()) { + remoteMetaServer.close(); + }
🧹 Nitpick comments (5)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (5)
270-272: LGTM: return a mutable list by default to enable subclass extension.This change prevents UnsupportedOperationException when subclasses call
super.customSessionServerHandlers().add(...). Consider adding a brief Javadoc noting that subclasses may mutate and should return a new collection per call.
253-255: Prefer zero-length array in toArray().Use
new ChannelHandler[0](JDK 11+) for simpler, typically better-performing allocation.- mergedSessionServerHandlers.toArray( - new ChannelHandler[mergedSessionServerHandlers.size()])); + mergedSessionServerHandlers.toArray(new ChannelHandler[0]));
281-283: Apply the same toArray() simplification here.- dataServerHandlers.toArray(new ChannelHandler[dataServerHandlers.size()])); + dataServerHandlers.toArray(new ChannelHandler[0]));
303-305: Apply the same toArray() simplification here.- metaServerHandlers.toArray(new ChannelHandler[metaServerHandlers.size()])); + metaServerHandlers.toArray(new ChannelHandler[0]));
341-344: Apply the same toArray() simplification here.- remoteMetaServerHandlers.toArray( - new ChannelHandler[remoteMetaServerHandlers.size()])); + remoteMetaServerHandlers.toArray(new ChannelHandler[0]));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
| import javax.annotation.Resource; | ||
| import javax.ws.rs.Path; | ||
| import javax.ws.rs.ext.Provider; | ||
| import java.lang.annotation.Annotation; | ||
| import java.util.*; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Avoid wildcard import; align import order with project Checkstyle.
Replace java.util.* with explicit imports and keep import groups ordered consistently to prevent future CI/lint churn.
-import javax.annotation.Resource;
-import javax.ws.rs.Path;
-import javax.ws.rs.ext.Provider;
-import java.lang.annotation.Annotation;
-import java.util.*;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Resource;
+import javax.ws.rs.Path;
+import javax.ws.rs.ext.Provider;
+import java.lang.annotation.Annotation;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;📝 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.
| import javax.annotation.Resource; | |
| import javax.ws.rs.Path; | |
| import javax.ws.rs.ext.Provider; | |
| import java.lang.annotation.Annotation; | |
| import java.util.*; | |
| import java.util.concurrent.TimeUnit; | |
| import java.util.concurrent.atomic.AtomicBoolean; | |
| import javax.annotation.Resource; | |
| import javax.ws.rs.Path; | |
| import javax.ws.rs.ext.Provider; | |
| import java.lang.annotation.Annotation; | |
| import java.util.ArrayList; | |
| import java.util.Collection; | |
| import java.util.Date; | |
| import java.util.List; | |
| import java.util.Map; | |
| import java.util.concurrent.TimeUnit; | |
| import java.util.concurrent.atomic.AtomicBoolean; |
hui-cha
force-pushed
the
feature/replace_emptylist
branch
from
bd0f500 to
0bc747d
Compare
2 participants
修复下这个问题
#372 (comment)
Summary by CodeRabbit
Refactor
Style