Release 650 #351
Merged
Release 650 #351
Conversation
This file contains 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
huanglongchao
force-pushed
the
release_650
branch
from
9c603a9
to
d63f6e3
Compare
huanglongchao
force-pushed
the
release_650
branch
from
d63f6e3
to
8518b27
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #351 +/- ##
============================================
+ Coverage 76.65% 76.73% +0.08%
Complexity 48 48
============================================
Files 495 495
Lines 18506 18591 +85
Branches 1805 1822 +17
============================================
+ Hits 14185 14266 +81
+ Misses 3508 3503 -5
- Partials 813 822 +9
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
huanglongchao
force-pushed
the
release_650
branch
2 times, most recently
from
20d4567
to
97e5e5d
Compare
huanglongchao
force-pushed
the
release_650
branch
6 times, most recently
from
ad8001e
to
8708c34
Compare
huanglongchao
force-pushed
the
release_650
branch
from
8708c34
to
0b55641
Compare
huanglongchao
force-pushed
the
release_650
branch
from
7fa75f0
to
d93635f
Compare
WalkthroughThe recent update brings enhancements across various components, focusing on refining server selection algorithms, improving error handling, and introducing new features for better resource management. Notable changes include optimizing server connection strategies, refining server weight handling, and enhancing system resilience and configurability. The version bump signifies meaningful progress in the software's evolution. Changes
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (30)
client/all/pom.xmlis excluded by:!**/*.xmlclient/api/pom.xmlis excluded by:!**/*.xmlclient/impl/pom.xmlis excluded by:!**/*.xmlclient/log/pom.xmlis excluded by:!**/*.xmlclient/pom.xmlis excluded by:!**/*.xmlcore/pom.xmlis excluded by:!**/*.xmlpom.xmlis excluded by:!**/*.xmlserver/common/model/pom.xmlis excluded by:!**/*.xmlserver/common/pom.xmlis excluded by:!**/*.xmlserver/common/util/pom.xmlis excluded by:!**/*.xmlserver/distribution/all/pom.xmlis excluded by:!**/*.xmlserver/distribution/pom.xmlis excluded by:!**/*.xmlserver/pom.xmlis excluded by:!**/*.xmlserver/remoting/api/pom.xmlis excluded by:!**/*.xmlserver/remoting/bolt/pom.xmlis excluded by:!**/*.xmlserver/remoting/http/pom.xmlis excluded by:!**/*.xmlserver/remoting/pom.xmlis excluded by:!**/*.xmlserver/server/data/pom.xmlis excluded by:!**/*.xmlserver/server/integration/pom.xmlis excluded by:!**/*.xmlserver/server/meta/pom.xmlis excluded by:!**/*.xmlserver/server/pom.xmlis excluded by:!**/*.xmlserver/server/session/pom.xmlis excluded by:!**/*.xmlserver/server/shared/pom.xmlis excluded by:!**/*.xmlserver/store/api/pom.xmlis excluded by:!**/*.xmlserver/store/jdbc/pom.xmlis excluded by:!**/*.xmlserver/store/jdbc/src/main/resources/h2-mapper/interface_apps_index.xmlis excluded by:!**/*.xmlserver/store/jdbc/src/main/resources/mapper/interface_apps_index.xmlis excluded by:!**/*.xmlserver/store/jraft/pom.xmlis excluded by:!**/*.xmlserver/store/pom.xmlis excluded by:!**/*.xmltest/pom.xmlis excluded by:!**/*.xml
Files selected for processing (40)
- .github/workflows/unit-test.yml (2 hunks)
- VERSION (1 hunks)
- client/impl/src/main/java/com/alipay/sofa/registry/client/provider/DefaultServerManager.java (2 hunks)
- client/impl/src/main/java/com/alipay/sofa/registry/client/provider/DefaultServerNode.java (3 hunks)
- client/impl/src/main/java/com/alipay/sofa/registry/client/remoting/ClientConnection.java (1 hunks)
- client/impl/src/main/java/com/alipay/sofa/registry/client/remoting/ServerNode.java (1 hunks)
- client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultServerManagerTest.java (2 hunks)
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1 hunks)
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/metaserver/nodes/SessionNode.java (2 hunks)
- server/common/model/src/test/java/com/alipay/sofa/registry/common/model/metaserver/nodes/NodeTest.java (1 hunks)
- server/common/model/src/test/java/com/alipay/sofa/registry/common/model/metaserver/rpc/DataCenterNodesTest.java (1 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/config/MetaServerConfig.java (1 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/config/MetaServerConfigBean.java (2 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/cleaner/InterfaceAppsIndexCleaner.java (5 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/lease/filter/DefaultForbiddenServerManager.java (2 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/lease/session/DefaultSessionServerManager.java (1 hunks)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MetaCenterResource.java (3 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/cleaner/InterfaceAppsIndexCleanerTest.java (3 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/lease/session/DefaultSessionServerManagerTest.java (8 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/metaserver/impl/DefaultCurrentDcMetaServerTest.java (1 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/remoting/session/DefaultSessionServerServiceTest.java (3 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/MetaCenterResourceTest.java (3 hunks)
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/RegistryCoreOpsResourceTest.java (2 hunks)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/node/service/MetaServerServiceImpl.java (4 hunks)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionDigestResource.java (2 hunks)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionOpenResource.java (4 hunks)
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/AbstractSessionServerTestBase.java (1 hunks)
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/node/service/MetaServerServiceImplTest.java (2 hunks)
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchStopPushServiceTest.java (1 hunks)
- server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/meta/AbstractMetaServerService.java (1 hunks)
- server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/meta/MetaServerService.java (2 hunks)
- server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/BaseStopPushService.java (1 hunks)
- server/server/shared/src/test/java/com/alipay/sofa/registry/server/shared/meta/MetaServerServiceTest.java (3 hunks)
- server/server/shared/src/test/java/com/alipay/sofa/registry/server/shared/util/NodeUtilsTest.java (1 hunks)
- server/store/api/src/main/java/com/alipay/sofa/registry/store/api/repository/InterfaceAppsRepository.java (2 hunks)
- server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/mapper/InterfaceAppsIndexMapper.java (2 hunks)
- server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepository.java (1 hunks)
- server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepositoryTest.java (2 hunks)
- server/store/jraft/src/main/java/com/alipay/sofa/registry/jraft/repository/impl/InterfaceAppsRaftRepository.java (2 hunks)
- test/src/test/java/com/alipay/sofa/registry/test/metadata/InterfaceAppsJdbcRepositoryTest.java (2 hunks)
Additional comments: 73
VERSION (1)
- 1-1: The version update from 6.4.0 to 6.5.0 correctly follows semantic versioning principles, indicating backward-compatible new features or improvements.
.github/workflows/unit-test.yml (3)
- 16-16: Adding a
timeout-minutes: 15setting for thebuildjob is a good practice to prevent indefinitely running jobs, which can save CI resources and highlight issues with hanging tests or builds.- 27-32: Introducing a step to upload heap dump artifacts (
heap-dump) is beneficial for diagnosing memory-related issues in tests. Ensure that the path/tmp/*.hprofcorrectly captures the generated heap dumps.- 28-28: The condition
if: always()for theUpload heap dumpstep ensures that heap dumps are uploaded regardless of the job's success or failure, which is crucial for debugging issues in failing builds.client/impl/src/main/java/com/alipay/sofa/registry/client/remoting/ServerNode.java (1)
- 57-62: The addition of the
getWeight()method to theServerNodeinterface aligns with the PR objectives for enhancing load balancing and server connection logic. The method is properly documented.server/server/shared/src/test/java/com/alipay/sofa/registry/server/shared/util/NodeUtilsTest.java (1)
- 31-32: The addition of an integer parameter
0to theSessionNodeconstructor in the test method aligns with the PR objectives related to adding session weight information. This change ensures that the test reflects the updated constructor signature.server/store/api/src/main/java/com/alipay/sofa/registry/store/api/repository/InterfaceAppsRepository.java (1)
- 43-43: The addition of the
cleanDeletedmethod to theInterfaceAppsRepositoryinterface aligns with the PR objectives related to introducing a metadata cleanup mechanism. The method is properly documented, and its parameters are clear and relevant to its purpose.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchStopPushServiceTest.java (1)
- 40-40: The change in assertion for
isStopPushSwitch()fromtruetofalsein thetest()method likely reflects an update in the expected behavior of theFetchStopPushService. Ensure this change aligns with the intended functionality.server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/mapper/InterfaceAppsIndexMapper.java (1)
- 60-63: The addition of the
cleanDeletedmethod to theInterfaceAppsIndexMapperinterface supports the metadata cleanup mechanism mentioned in the PR objectives. The method is properly documented, and its parameters are clear and relevant to its purpose.server/common/model/src/test/java/com/alipay/sofa/registry/common/model/metaserver/rpc/DataCenterNodesTest.java (1)
- 34-35: The update to include an additional parameter in the
SessionNodeconstructor call in thetest()method aligns with the PR objectives related to adding session weight information. This change ensures that the test reflects the updated constructor signature.server/common/model/src/main/java/com/alipay/sofa/registry/common/model/metaserver/nodes/SessionNode.java (3)
- 30-32: The addition of the
weightfield in theSessionNodeclass is correctly implemented and follows Java conventions.- 40-43: The modification to the constructor to include and initialize the
weightfield is correctly implemented.- 68-70: The
getWeightmethod is correctly implemented. The design choice to not include a setter method suggests theweightfield is intended to be immutable, which is appropriate for this context.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/config/MetaServerConfig.java (1)
- 82-83: The addition of the
getInterfaceMaxRemovemethod to theMetaServerConfiginterface is correctly implemented and follows Java conventions.server/store/jraft/src/main/java/com/alipay/sofa/registry/jraft/repository/impl/InterfaceAppsRaftRepository.java (1)
- 56-59: The
cleanDeletedmethod appears to be a placeholder or not yet implemented, as it currently returns0. Please ensure its functionality is correctly implemented according to its intended purpose.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/MetaCenterResourceTest.java (1)
- 64-68: The
testInterfaceStartCleanermethod is correctly implemented, with proper setup, action, and assertion phases. The use of a mockInterfaceAppsIndexCleaneris appropriate for isolating the test case.server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/meta/MetaServerService.java (1)
- 76-77: The addition of the
getSessionNodeWithConnNumListmethod to theMetaServerServiceinterface is correctly implemented and follows Java conventions.test/src/test/java/com/alipay/sofa/registry/test/metadata/InterfaceAppsJdbcRepositoryTest.java (2)
- 23-23: The change to import all classes from the
java.utilpackage, instead of specific classes, is a style preference. Ensure it doesn't introduce naming conflicts or ambiguity.- 82-82: The addition of the
cleanDeletedmethod call in thebatchSaveTestmethod is logical for testing cleanup functionality. Ensure its impact on the test is as intended.server/common/model/src/test/java/com/alipay/sofa/registry/common/model/metaserver/nodes/NodeTest.java (1)
- 54-56: The modification to the
testSessionNodemethod to include an additional parameter in theSessionNodeconstructor calls is correctly implemented, reflecting the changes in theSessionNodeclass.client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultServerManagerTest.java (2)
- 20-20: The change to import
Matchers.*instead of individualMatchersmethods is a style preference. Ensure it doesn't introduce naming conflicts or ambiguity.- 69-99: The
initServerListCompatibletest method is correctly implemented, effectively testing the server list retrieval logic, including compatibility handling. The use of mock static methods and conditional responses is appropriate for isolating the test case.client/impl/src/main/java/com/alipay/sofa/registry/client/provider/DefaultServerNode.java (2)
- 39-39: The addition of the
WEIGHT_KEYconstant improves code readability and maintainability by avoiding hard-coded strings. Good practice!- 86-100: The modifications to the
getWeight()method correctly implement the retrieval and parsing of the weight property using theWEIGHT_KEYconstant. HandlingNumberFormatExceptionby returning a default weight of0ensures the method's robustness. Well done!server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionOpenResource.java (4)
- 19-21: The added imports are necessary for the new functionality introduced, specifically for handling session nodes and managing exchange operations. This is a good practice to ensure the code's modularity and readability.
- 71-83: The addition of the
getSessionServerListWithConnNummethod enhances the system's observability by allowing users to query session servers along with their connection numbers. This is a valuable feature for monitoring and load balancing purposes.- 85-90: The addition of the
getCurrentSessionConnNummethod provides essential functionality for monitoring the number of active connections in the current session. This is a useful addition for system health checks and load management.- 120-133: The addition of the
getSessionServersWithConnNumprivate method supports the new functionality by providing the necessary data retrieval and formatting logic. It's well-implemented and serves a clear purpose in enhancing system observability.client/impl/src/main/java/com/alipay/sofa/registry/client/provider/DefaultServerManager.java (1)
- 100-113: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [93-110]
The introduction of a fallback mechanism in the
syncServerList()method improves the robustness of the server list synchronization process. It ensures that the system can still retrieve server information even if the preferred query method is not supported. This is a sensible approach to handle potential compatibility issues.server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepositoryTest.java (2)
- 35-35: Simplifying import statements by using a wildcard import for
java.util.*is a common practice in test classes and helps reduce clutter. This change is acceptable.- 132-136: The addition of the
testCleaned()method is a good practice for ensuring the functionality related to cleaning deleted entries works as expected. It's important to continuously expand test coverage to maintain and improve code quality.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MetaCenterResource.java (2)
- 65-73: The addition of the
interfaceAppsIndexCleanmethod provides a necessary administrative function to manage the lifecycle of interface apps index data. This is a valuable addition for system maintenance and data hygiene.- 90-103: Providing the ability to enable or disable the interface apps cleaner dynamically enhances the system's manageability. This method allows administrators to control this aspect based on current needs or system state, which is crucial for operational flexibility.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/node/service/MetaServerServiceImpl.java (4)
- 56-56: The injection of
Exchange boltExchangeis necessary for the new functionality introduced, specifically for calculating server weight based on active connections. This is a good practice to ensure the code's modularity and readability.- 117-122: The addition of the
getWeight()method enhances the system's ability to dynamically adjust based on load by calculating the server weight based on the number of active connections. This is a useful addition for load balancing and resource allocation.- 155-158: The addition of the setter for
boltExchangeallows for dependency injection, enhancing testing and configuration flexibility. This is a good practice for maintaining modularity and ease of testing.- 111-114: Updating the
createNode()method to incorporate server weight calculation ensures that the node creation process takes into account the dynamic aspects of server load. This is a logical step following the introduction of thegetWeight()method and enhances the system's adaptability.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/cleaner/InterfaceAppsIndexCleanerTest.java (6)
- 21-35: The imports introduced here are necessary for the new test methods and dependencies added to the
InterfaceAppsIndexCleanerTest. These changes align with the PR objectives related to enhancing the metadata cleanup mechanism.- 48-68: The setup method
beforeTesthas been significantly expanded to initialize and mock dependencies forInterfaceAppsIndexCleaner. This setup is crucial for the new test methods that verify the cleaner's functionality. It's well-structured and provides a comprehensive environment for testing the cleaner's behavior.- 70-73: The
afterTestmethod ensures that resources are properly closed after tests, preventing potential resource leaks during test execution. This is a good practice, especially when dealing with external resources or services in tests.- 107-117: The
testCleanInterfacemethod tests the cleaning functionality of theInterfaceAppsIndexCleaner. It mocks the necessary components and verifies that the cleaner can successfully clean deleted interfaces. This test method directly relates to the PR's objective of introducing a cleanup mechanism for metadata.- 119-129: The
testEnablemethod tests the enabling functionality of theInterfaceAppsIndexCleaner. It ensures that the cleaner can be enabled and performs its duties as expected. This method is essential for validating the cleaner's ability to be controlled programmatically, which is crucial for its integration into the system.- 131-137: The
testEnableThrowExmethod tests the cleaner's behavior when an exception is thrown during the enabling process. This test ensures that the cleaner can handle exceptions gracefully, which is important for the robustness of the system. It's a good practice to test exceptional cases to ensure the system's stability under various conditions.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/RegistryCoreOpsResourceTest.java (2)
- 24-30: The imports introduced here are necessary for the new test methods added to the
RegistryCoreOpsResourceTest. These changes align with the PR objectives related to server connection and weight handling improvements.- 128-143: The
testDistinguishServerGroupmethod tests the functionality of distinguishing server groups based on different entities such asProcessId,DataNode,MetaNode, andSessionNode. This test method directly relates to the PR's objective of improving load balancing and server connection logic by handling different node types appropriately.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/node/service/MetaServerServiceImplTest.java (2)
- 28-28: The import of
BoltExchangeis necessary for the new initialization added to theMetaServerServiceImplTest. This change aligns with the PR objectives related to server connection improvements.- 141-141: The initialization of
BoltExchangein theinitmethod is a crucial addition for testing theMetaServerServiceImpl's functionality related to server connections. This ensures that the component is properly tested in an environment that closely mimics the production setup.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/metaserver/impl/DefaultCurrentDcMetaServerTest.java (1)
- 85-86: The modification to include an additional parameter in the
SessionNodeconstructor calls within thetestGetSessionServersmethod aligns with the PR's objective of adding session weight to thesessionNodeheartbeat information. This change ensures that the tests reflect the updatedSessionNodestructure.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/lease/session/DefaultSessionServerManagerTest.java (2)
- 22-22: The addition of the
Leaseimport is necessary for the modifications made to theSessionNodeconstructor calls in various test methods. This change supports the testing of session weight information enhancements.- 70-70: The modifications to include an additional parameter
0in theSessionNodeconstructor calls across various test methods align with the PR's objective of adding session weight to thesessionNodeheartbeat information. These changes ensure that the tests accurately reflect the updatedSessionNodestructure and its usage in the system.Also applies to: 86-87, 105-106, 115-115, 136-136, 147-148, 172-176
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/lease/filter/DefaultForbiddenServerManager.java (2)
- 29-31: The imports for
DataNode,MetaNode, andSessionNodeare necessary for the modifications made to theallowSelectmethod. These changes align with the PR objectives related to server connection and weight handling improvements.- 97-105: The modification to the
allowSelectmethod to handle different node types based on the lease renewal is a crucial enhancement. It ensures that the system can distinguish betweenSessionNode,DataNode, andMetaNodewhen determining if a node is allowed to be selected. This change directly supports the PR's objective of improving load balancing and server connection logic by considering node types.server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
- 142-146: The addition of
INTERFACE_APP_CLEANER_ENABLED_DATA_IDfollows the established pattern for defining constants and is consistent with the naming conventions used throughout the file. This change looks good.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/remoting/session/DefaultSessionServerServiceTest.java (3)
- 94-96: The addition of the extra parameter
0toSessionNodeconstructor calls is consistent and necessary to reflect changes in theSessionNodestructure. This change looks good.- 117-119: The addition of the extra parameter
0toSessionNodeconstructor calls in this section is consistent with the changes made elsewhere in the file. This ensures the tests align with the updatedSessionNodestructure.- 157-159: The consistent addition of the extra parameter
0toSessionNodeconstructor calls in this section aligns with the updatedSessionNodestructure. This change is appropriate and necessary for the tests.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/cleaner/InterfaceAppsIndexCleaner.java (9)
- 53-53: The field
DEFAULT_ENABLEDis set tofalseby default, which controls whether the cleaner is enabled.Ensure that the default behavior aligns with the system's expectations and that any necessary configuration is documented for enabling this feature.
- 75-81: The
initmethod initializes theconsecutiveSuccessfield and registers the current instance as a listener tometaLeaderService.Consider adding error handling or logging in case the registration fails or the initialization encounters issues.
- 98-100: The cleaner thread is started in the
startmethod.Ensure that there's logic in place to prevent multiple invocations of
startfrom creating multiple cleaner threads, which could lead to unexpected behavior.
- 141-143: The
consecutiveSuccess.fail()method is called within a catch block.It's good practice to track consecutive failures as it can help in identifying issues with the cleanup process. Ensure that there's a mechanism to alert or take corrective action after a certain threshold of consecutive failures.
- 148-156: The
isEnabledmethod checks if the cleaner is enabled by querying aprovideDataService.This dynamic enable/disable feature is useful for operational flexibility. Ensure that the
provideDataServiceis reliable and that there's error handling for cases where the service might be unavailable.
- 158-163: The
dateBeforeNowmethod calculates a date that is a certain number of minutes before the current time.This method is useful for determining the cutoff time for cleaning up old entries. Ensure that the time calculation correctly accounts for time zone and daylight saving changes.
- 165-178: The
cleanupmethod performs the actual cleanup of outdated interface apps.Ensure that there's sufficient logging within this method to track what is being cleaned up and any errors that occur during the cleanup process.
- 189-199: The
setEnabledmethod allows dynamically enabling or disabling the cleaner.This method enhances operational flexibility by allowing the cleaner to be enabled or disabled without restarting the service. Ensure that changes made through this method are persisted and respected across service restarts.
- 202-210: The
becomeLeaderandloseLeadermethods clear theconsecutiveSuccesscounter upon leadership changes.Resetting the counter on leadership changes is a good practice to ensure that the state is consistent with the new role of the node. Ensure that other stateful components of the cleaner also reset appropriately upon leadership changes.
server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepository.java (1)
- 107-111: The
cleanDeletedmethod is added to clean deleted entries based on the default cluster ID.Ensure that the method is properly tested, especially with edge cases such as when there are no entries to clean or when the limit parameter is set to various values. Additionally, consider adding transaction management if multiple database operations need to be executed atomically.
client/impl/src/main/java/com/alipay/sofa/registry/client/remoting/ClientConnection.java (2)
- 128-138: The logic for selecting a server to connect to has been changed to implement the "Power of Two Choices" algorithm.
This change should improve load distribution among servers. Ensure that the algorithm's implementation correctly handles edge cases, such as when there's only one server in the list. Additionally, consider adding unit tests to validate the behavior of the new server selection logic.
- 146-146: The logging message has been updated to reflect the chosen server correctly.
It's good practice to log the outcome of significant operations like server selection for debugging and monitoring purposes. Ensure that the log level is appropriate and that sensitive information is not logged.
server/server/shared/src/test/java/com/alipay/sofa/registry/server/shared/meta/MetaServerServiceTest.java (3)
- 124-126: The extra parameter
0added to the constructor calls ofSessionNodeinstances in the test methodtestHandleHeartbeatRespis consistent with the updated constructor signature.Ensure that the added parameter (
0in this case) is correctly representing the intended value for the new parameter in the context of these tests. It's also important to verify that all other test cases that instantiateSessionNodeobjects are updated accordingly.
- 155-157: A new assertion for the size of the
nodeslist is added in the test methodtestHandleHeartbeatResp.Adding assertions to verify the size of the list is a good practice to ensure that the expected number of nodes is processed. Ensure that similar assertions are added to other relevant test cases to maintain consistency and thoroughness in testing.
- 241-243: The extra parameter
0added to the constructor calls ofSessionNodeinstances in the test methodtestRenewNodeis consistent with the updated constructor signature.As with the previous comment, ensure that the added parameter (
0in this case) is correctly representing the intended value for the new parameter in the context of these tests. Review all test cases for consistency in handling the updated constructor signature.server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/meta/AbstractMetaServerService.java (1)
- 369-380: Adding Javadoc comments to the
getSessionNodeWithConnNumListmethod would be beneficial to explain its purpose, parameters, and expected behavior, enhancing the code's readability and maintainability.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/config/MetaServerConfigBean.java (1)
- 78-78: The addition of the
interfaceMaxRemovefield and its corresponding getter and setter methods are correctly implemented and follow Java bean conventions. This change is consistent with the existing code structure and naming conventions.
Comment on lines
+886
to
+889
| @Override | ||
| public int cleanDeleted(Date beforeTime, int limit) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The implementation of the cleanDeleted method in the InMemoryInterfaceAppsRepository class currently returns 0 without performing any operations. This might be a placeholder implementation. If the intention is to actually clean up deleted entries based on the beforeTime and limit parameters, the method's logic needs to be implemented to reflect this functionality. Otherwise, if this is intended for future implementation or if the method is not expected to have any effect in this context (e.g., because it's a test or in-memory implementation), it would be helpful to add a comment explaining this to avoid confusion for future maintainers.
Would you like assistance in implementing this method, or should we document the current behavior for clarity?
Comment on lines
883
to
893
| @Override | ||
| public void renew(String interfaceName, String appName) {} | ||
|
|
||
| @Override | ||
| public int cleanDeleted(Date beforeTime, int limit) { | ||
| return 0; | ||
| } | ||
|
|
||
| @Override | ||
| public void startSynced() {} | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [207-207]
The use of ServerSocket without encryption could expose the system to security vulnerabilities, especially if sensitive data is being transmitted. Consider using SSLSocket created by SSLSocketFactory or SSLServerSocketFactory for encrypted communication. This is particularly important if the test environment closely mirrors production or if the tests involve sensitive data. However, if this socket usage is strictly for local testing and does not involve sensitive data, the risk might be mitigated. Still, it's a good practice to align test environments as closely as possible with production standards, including security practices.
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
Consider avoiding wildcard imports (import java.util.*;) for clarity and to prevent potential naming conflicts. Explicit imports are generally preferred in Java for better readability and maintainability.
Comment on lines
302
to
328
| @GET | ||
| @Path("{type}/getDataInfoIdListGroupByCount") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public List<Map.Entry<String, Integer>> getDataInfoIdListGroupByCount( | ||
| final @PathParam("type") String type) { | ||
| List<Map.Entry<String, Integer>> list = new ArrayList<>(); | ||
| String inputType = type.toUpperCase(); | ||
| switch (inputType) { | ||
| case SUB: | ||
| sessionDataStore | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| case WAT: | ||
| sessionWatchers | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| default: | ||
| sessionInterests | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| } | ||
| list.sort(Collections.reverseOrder(Map.Entry.comparingByValue())); | ||
| return list; | ||
| } |
There was a problem hiding this comment.
Consider refactoring the getDataInfoIdListGroupByCount method to reduce duplication in the switch statement cases. Additionally, adding Javadoc comments to explain the method's purpose, parameters, and return value would enhance readability and maintainability. Lastly, using generic types for Map and Entry objects would improve type safety.
| @@ -32,7 +32,7 @@ public abstract class BaseStopPushService | |||
|
|
|||
| @Autowired private ProvideDataRepository provideDataRepository; | |||
|
|
|||
| private static final StopPushStorage INIT = new StopPushStorage(INIT_VERSION, true); | |||
| private static final StopPushStorage INIT = new StopPushStorage(INIT_VERSION, false); | |||
There was a problem hiding this comment.
Changing the default value of INIT from true to false alters the initial state of the stop push mechanism. Ensure this change aligns with the intended system behavior and that all dependent components are compatible with this new default state. It's crucial to conduct thorough testing to verify the impact of this change across the system.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (32)
- VERSION (1 hunks)
- client/all/pom.xml (1 hunks)
- client/api/pom.xml (1 hunks)
- client/impl/pom.xml (1 hunks)
- client/impl/src/main/java/com/alipay/sofa/registry/client/remoting/ClientConnection.java (1 hunks)
- client/log/pom.xml (1 hunks)
- client/pom.xml (1 hunks)
- core/pom.xml (1 hunks)
- pom.xml (1 hunks)
- server/common/model/pom.xml (1 hunks)
- server/common/pom.xml (1 hunks)
- server/common/util/pom.xml (1 hunks)
- server/distribution/all/pom.xml (1 hunks)
- server/distribution/pom.xml (1 hunks)
- server/pom.xml (1 hunks)
- server/remoting/api/pom.xml (1 hunks)
- server/remoting/bolt/pom.xml (1 hunks)
- server/remoting/http/pom.xml (1 hunks)
- server/remoting/pom.xml (1 hunks)
- server/server/data/pom.xml (1 hunks)
- server/server/integration/pom.xml (1 hunks)
- server/server/meta/pom.xml (1 hunks)
- server/server/pom.xml (1 hunks)
- server/server/session/pom.xml (1 hunks)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionDigestResource.java (2 hunks)
- server/server/shared/pom.xml (1 hunks)
- server/store/api/pom.xml (1 hunks)
- server/store/jdbc/pom.xml (1 hunks)
- server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepository.java (2 hunks)
- server/store/jraft/pom.xml (1 hunks)
- server/store/pom.xml (1 hunks)
- test/pom.xml (2 hunks)
Files skipped from review due to trivial changes (28)
- VERSION
- client/all/pom.xml
- client/api/pom.xml
- client/impl/pom.xml
- client/log/pom.xml
- client/pom.xml
- core/pom.xml
- pom.xml
- server/common/model/pom.xml
- server/common/pom.xml
- server/common/util/pom.xml
- server/distribution/all/pom.xml
- server/distribution/pom.xml
- server/pom.xml
- server/remoting/api/pom.xml
- server/remoting/bolt/pom.xml
- server/remoting/http/pom.xml
- server/remoting/pom.xml
- server/server/data/pom.xml
- server/server/integration/pom.xml
- server/server/meta/pom.xml
- server/server/pom.xml
- server/server/session/pom.xml
- server/server/shared/pom.xml
- server/store/api/pom.xml
- server/store/jdbc/pom.xml
- server/store/jraft/pom.xml
- server/store/pom.xml
Additional comments not posted (4)
test/pom.xml (2)
8-8: Version update to 6.5.1 aligns with the overall project versioning strategy.
125-125: Updated JVM arguments to increase memory limits and enable heap dumps on out-of-memory errors. Ensure these changes do not negatively impact the test execution times.server/store/jdbc/src/main/java/com/alipay/sofa/registry/jdbc/repository/impl/InterfaceAppsJdbcRepository.java (2)
107-111: AddedcleanDeletedmethod to remove old entries from the database based on a timestamp and limit. This method enhances data hygiene and storage management.
228-231: Modified conditional check to ensure that only entries with a newer version or different apps are processed. This change improves data consistency and efficiency.
Comment on lines
+128
to
+142
| int choosed = 0; | ||
| for (int i = 0; i < serverNodes.size(); i++) { | ||
| try { | ||
| connection = connect(serverNode); | ||
| // Power of Two Choices | ||
| if (serverNodes.size() > 1) { | ||
| choosed = | ||
| serverNodes.get(i).getWeight() | ||
| > serverNodes.get((i + 1) % serverNodes.size()).getWeight() | ||
| ? (i + 1) % serverNodes.size() | ||
| : i; | ||
| } | ||
| connection = connect(serverNodes.get(choosed)); | ||
| if (null != connection && connection.isFine()) { | ||
| resetRegister(); | ||
| LOGGER.info("[Connect] Successfully connected to server: {}", serverNode); | ||
| LOGGER.info("[Connect] Successfully connected to server: {}", serverNodes.get(choosed)); |
There was a problem hiding this comment.
Implemented the Power of Two Choices algorithm for server node selection to improve connection efficiency. Consider adding a comment explaining the algorithm for future maintainability.
+ // Power of Two Choices algorithm: randomly selects two server nodes and chooses the one with the higher weight.Also applies to: 150-150
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.
Suggested change
| int choosed = 0; | |
| for (int i = 0; i < serverNodes.size(); i++) { | |
| try { | |
| connection = connect(serverNode); | |
| // Power of Two Choices | |
| if (serverNodes.size() > 1) { | |
| choosed = | |
| serverNodes.get(i).getWeight() | |
| > serverNodes.get((i + 1) % serverNodes.size()).getWeight() | |
| ? (i + 1) % serverNodes.size() | |
| : i; | |
| } | |
| connection = connect(serverNodes.get(choosed)); | |
| if (null != connection && connection.isFine()) { | |
| resetRegister(); | |
| LOGGER.info("[Connect] Successfully connected to server: {}", serverNode); | |
| LOGGER.info("[Connect] Successfully connected to server: {}", serverNodes.get(choosed)); | |
| int choosed = 0; | |
| for (int i = 0; i < serverNodes.size(); i++) { | |
| try { | |
| // Power of Two Choices | |
| // Power of Two Choices algorithm: randomly selects two server nodes and chooses the one with the higher weight. | |
| if (serverNodes.size() > 1) { | |
| choosed = | |
| serverNodes.get(i).getWeight() | |
| > serverNodes.get((i + 1) % serverNodes.size()).getWeight() | |
| ? (i + 1) % serverNodes.size() | |
| : i; | |
| } | |
| connection = connect(serverNodes.get(choosed)); | |
| if (null != connection && connection.isFine()) { | |
| resetRegister(); | |
| LOGGER.info("[Connect] Successfully connected to server: {}", serverNodes.get(choosed)); |
Comment on lines
+302
to
+328
| @GET | ||
| @Path("{type}/getDataInfoIdListGroupByCount") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public List<Map.Entry<String, Integer>> getDataInfoIdListGroupByCount( | ||
| final @PathParam("type") String type) { | ||
| List<Map.Entry<String, Integer>> list = new ArrayList<>(); | ||
| String inputType = type.toUpperCase(); | ||
| switch (inputType) { | ||
| case PUB: | ||
| sessionDataStore | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| case WAT: | ||
| sessionWatchers | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| default: | ||
| sessionInterests | ||
| .getDatas() | ||
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | ||
| break; | ||
| } | ||
| list.sort(Collections.reverseOrder(Map.Entry.comparingByValue())); | ||
| return list; | ||
| } |
There was a problem hiding this comment.
Added a new REST endpoint to group data IDs by count based on type. This method provides valuable insights for monitoring and analysis. Consider adding a comment explaining the method's purpose and usage.
+ // Endpoint to group data IDs by count: aggregates data based on type and sorts them in descending order by count.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.
Suggested change
| @GET | |
| @Path("{type}/getDataInfoIdListGroupByCount") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public List<Map.Entry<String, Integer>> getDataInfoIdListGroupByCount( | |
| final @PathParam("type") String type) { | |
| List<Map.Entry<String, Integer>> list = new ArrayList<>(); | |
| String inputType = type.toUpperCase(); | |
| switch (inputType) { | |
| case PUB: | |
| sessionDataStore | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| case WAT: | |
| sessionWatchers | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| default: | |
| sessionInterests | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| } | |
| list.sort(Collections.reverseOrder(Map.Entry.comparingByValue())); | |
| return list; | |
| } | |
| @GET | |
| @Path("{type}/getDataInfoIdListGroupByCount") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public List<Map.Entry<String, Integer>> getDataInfoIdListGroupByCount( | |
| final @PathParam("type") String type) { | |
| // Endpoint to group data IDs by count: aggregates data based on type and sorts them in descending order by count. | |
| List<Map.Entry<String, Integer>> list = new ArrayList<>(); | |
| String inputType = type.toUpperCase(); | |
| switch (inputType) { | |
| case PUB: | |
| sessionDataStore | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| case WAT: | |
| sessionWatchers | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| default: | |
| sessionInterests | |
| .getDatas() | |
| .forEach((key, value) -> list.add(new AbstractMap.SimpleEntry<>(key, value.size()))); | |
| break; | |
| } | |
| list.sort(Collections.reverseOrder(Map.Entry.comparingByValue())); | |
| return list; | |
| } |
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.
Motivation:
Result:
#348
#350
#349
#347
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation