-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update provider to null #1452
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
core-impl/client/src/main/java/com/alipay/sofa/rpc/client/AbstractCluster.java (1)
Line range hint
232-257
: Consistent improvement in handling empty provider groupsThe changes in the
updateAllProviders
method align with the improvements made in theupdateProviders
method. The additional check forCommonUtils.isNotEmpty(currentProviderList)
before logging a warning and closing transports prevents unnecessary actions when the provider list becomes empty.For consistency with the
updateProviders
method, consider introducing apreviouslyEmpty
variable here as well. This would make the logic more explicit and easier to understand.Consider refactoring the code as follows for improved consistency:
if (count == 0) { Collection<ProviderInfo> currentProviderList = currentProviderList(); + boolean previouslyEmpty = CommonUtils.isEmpty(currentProviderList); addressHolder.updateAllProviders(providerGroups); - if (CommonUtils.isNotEmpty(currentProviderList)) { + if (!previouslyEmpty) { if (LOGGER.isWarnEnabled(consumerConfig.getAppName())) { LOGGER.warnWithApp(consumerConfig.getAppName(), "Provider list is emptied, may be all " + "providers has been closed, or this consumer has been add to blacklist"); - closeTransports(); } + closeTransports(); } } else { // ... (rest of the code remains unchanged) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- core-impl/client/src/main/java/com/alipay/sofa/rpc/client/AbstractCluster.java (1 hunks)
- core-impl/client/src/test/java/com/alipay/sofa/rpc/client/ClusterProviderUpdateTest.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
core-impl/client/src/test/java/com/alipay/sofa/rpc/client/ClusterProviderUpdateTest.java (2)
1-40
: LGTM: File structure and imports are well-organized.The file has appropriate license information, necessary imports, and is correctly structured. The class name and package are appropriate for its purpose.
1-194
: Overall assessment: Good start, but room for improvementThis new test file for AbstractCluster is a valuable addition to the test suite. It covers basic scenarios for updating providers and aligns with the PR objectives. However, there are several areas where it can be enhanced:
- Consider using @BeforeClass for test setup instead of static initialization.
- Expand the test coverage in testUpdateProvider to include more scenarios and stronger assertions.
- Enhance the TestUseConnectionHolder class to be more versatile and provide better support for detailed testing.
These improvements will result in a more robust and comprehensive test suite, ensuring better code quality and reducing the likelihood of future regressions.
core-impl/client/src/main/java/com/alipay/sofa/rpc/client/AbstractCluster.java (2)
213-228
: Improved handling of empty provider groupsThe changes in the
updateProviders
method address the issue mentioned in the PR objectives. The introduction of thepreviouslyEmpty
variable and the refactored logic prevent unnecessary warnings and connection closures when the provider list transitions from non-empty to empty. This should help reduce excessive warning logs in the scenarios described in the linked issue #1442.The updated event posting logic also ensures that the
ProviderInfoUpdateEvent
is triggered with the correct information, addressing the swallow copy problem mentioned in the comment.
Line range hint
1-1009
: Overall improvement in provider managementThe changes made to the
updateProviders
andupdateAllProviders
methods in this file significantly improve the handling of empty provider groups. These modifications address the issues outlined in the PR objectives and the linked issue #1442, particularly:
- Preventing unnecessary warning logs when provider lists become empty.
- Avoiding redundant connection closures when transitioning from non-empty to empty provider lists.
- Ensuring correct event posting with updated provider information.
These improvements should lead to more efficient connection management and reduced log clutter in scenarios where provider services are restarted or temporarily unavailable.
The changes are focused and don't introduce any apparent regressions in other parts of the file. The overall structure and functionality of the
AbstractCluster
class remain intact, with the modifications enhancing its robustness in handling provider updates.
public class ClusterProviderUpdateTest { | ||
private static final AbstractCluster cluster; | ||
|
||
static { | ||
ConsumerBootstrap bootstrap = new ConsumerBootstrap(new ConsumerConfig()) { | ||
@Override | ||
public Object refer() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public void unRefer() { | ||
|
||
} | ||
|
||
@Override | ||
public Object getProxyIns() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public Cluster getCluster() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public List<ProviderGroup> subscribe() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public boolean isSubscribed() { | ||
return false; | ||
} | ||
}; | ||
|
||
cluster = new AbstractCluster(bootstrap) { | ||
@Override | ||
protected SofaResponse doInvoke(SofaRequest msg) throws SofaRpcException { | ||
return null; | ||
} | ||
}; | ||
|
||
cluster.addressHolder = new SingleGroupAddressHolder(null); | ||
cluster.connectionHolder = new TestUseConnectionHolder(null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using @BeforeClass instead of static initialization.
While the current setup allows for isolated testing of the cluster's behavior, using static initialization for test setup can lead to issues with test isolation and make it harder to reset the state between tests. Consider refactoring this to use JUnit's @BeforeClass
annotation for setup, which would provide better control over the test lifecycle.
Example refactoring:
public class ClusterProviderUpdateTest {
private static AbstractCluster cluster;
@BeforeClass
public static void setUp() {
ConsumerBootstrap bootstrap = new ConsumerBootstrap(new ConsumerConfig()) {
// ... (existing mock implementation)
};
cluster = new AbstractCluster(bootstrap) {
// ... (existing mock implementation)
};
cluster.addressHolder = new SingleGroupAddressHolder(null);
cluster.connectionHolder = new TestUseConnectionHolder(null);
}
// ... (rest of the test class)
}
This approach would maintain the same functionality while improving test isolation and maintainability.
@Test | ||
public void testUpdateProvider() { | ||
String groupName = "testUpdateProvider-Group"; | ||
List<ProviderInfo> providerList = Arrays.asList( | ||
ProviderHelper.toProviderInfo("127.0.0.1:12200"), | ||
ProviderHelper.toProviderInfo("127.0.0.1:12201"), | ||
ProviderHelper.toProviderInfo("127.0.0.1:12202")); | ||
ProviderGroup g = new ProviderGroup(groupName, providerList); | ||
cluster.updateProviders(g); | ||
|
||
Assert.assertEquals(cluster.currentProviderList().size(), providerList.size()); | ||
|
||
cluster.updateProviders(new ProviderGroup(groupName, null)); | ||
|
||
Assert.assertTrue(cluster.getAddressHolder().getProviderGroup(groupName).isEmpty()); | ||
Assert.assertEquals( 1, ((TestUseConnectionHolder)cluster.connectionHolder).calledCloseAllClientTransports.get()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance test coverage and assertions in testUpdateProvider method.
The current test covers basic scenarios, but consider the following improvements:
- Test with an empty provider list in addition to null.
- Verify the exact contents of the provider list after update, not just its size.
- Add a test case for updating with a different set of providers to ensure proper addition and removal.
- Consider testing edge cases, such as updating with duplicate providers.
Example of enhanced assertions:
@Test
public void testUpdateProvider() {
String groupName = "testUpdateProvider-Group";
List<ProviderInfo> providerList = Arrays.asList(
ProviderHelper.toProviderInfo("127.0.0.1:12200"),
ProviderHelper.toProviderInfo("127.0.0.1:12201"),
ProviderHelper.toProviderInfo("127.0.0.1:12202"));
ProviderGroup g = new ProviderGroup(groupName, providerList);
cluster.updateProviders(g);
List<ProviderInfo> currentProviders = cluster.currentProviderList();
Assert.assertEquals("Provider list size should match", providerList.size(), currentProviders.size());
Assert.assertTrue("All providers should be present", currentProviders.containsAll(providerList));
// Test with empty list
cluster.updateProviders(new ProviderGroup(groupName, Collections.emptyList()));
Assert.assertTrue("Provider group should be empty", cluster.getAddressHolder().getProviderGroup(groupName).isEmpty());
// Test with null list
cluster.updateProviders(new ProviderGroup(groupName, null));
Assert.assertTrue("Provider group should be empty", cluster.getAddressHolder().getProviderGroup(groupName).isEmpty());
Assert.assertEquals("closeAllClientTransports should be called twice", 2, ((TestUseConnectionHolder)cluster.connectionHolder).calledCloseAllClientTransports.get());
}
These enhancements will provide more comprehensive coverage and stronger assertions.
private static class TestUseConnectionHolder extends ConnectionHolder { | ||
Set<ProviderInfo> connections = new HashSet<>(); | ||
|
||
AtomicInteger calledCloseAllClientTransports = new AtomicInteger(); | ||
|
||
/** | ||
* 构造函数 | ||
* | ||
* @param consumerBootstrap 服务消费者配置 | ||
*/ | ||
protected TestUseConnectionHolder(ConsumerBootstrap consumerBootstrap) { | ||
super(consumerBootstrap); | ||
} | ||
|
||
@Override | ||
public void closeAllClientTransports(DestroyHook destroyHook) { | ||
calledCloseAllClientTransports.getAndIncrement(); | ||
} | ||
|
||
@Override | ||
public ConcurrentMap<ProviderInfo, ClientTransport> getAvailableConnections() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public List<ProviderInfo> getAvailableProviders() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public ClientTransport getAvailableClientTransport(ProviderInfo providerInfo) { | ||
return null; | ||
} | ||
|
||
@Override | ||
public boolean isAvailableEmpty() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public Collection<ProviderInfo> currentProviderList() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public void setUnavailable(ProviderInfo providerInfo, ClientTransport transport) { | ||
|
||
} | ||
|
||
@Override | ||
public void addProvider(ProviderGroup providerGroup) { | ||
throw new UnsupportedOperationException("not implemented"); | ||
} | ||
|
||
@Override | ||
public void removeProvider(ProviderGroup providerGroup) { | ||
throw new UnsupportedOperationException("not implemented"); | ||
|
||
} | ||
|
||
@Override | ||
public void updateProviders(ProviderGroup providerGroup) { | ||
for (ProviderInfo i : providerGroup.getProviderInfos()) { | ||
connections.add(i); | ||
} | ||
} | ||
|
||
@Override | ||
public void updateAllProviders(List<ProviderGroup> providerGroups) { | ||
|
||
} | ||
|
||
@Override | ||
public void destroy() { | ||
|
||
} | ||
|
||
@Override | ||
public void destroy(DestroyHook hook) { | ||
|
||
} | ||
|
||
@Override | ||
public void init() { | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance TestUseConnectionHolder for more comprehensive testing.
The current implementation of TestUseConnectionHolder is minimal and might not provide enough flexibility for future tests. Consider the following improvements:
- Implement the unimplemented methods with basic functionality or throw UnsupportedOperationException with a clear message.
- Add more tracking variables (similar to calledCloseAllClientTransports) for other important methods to allow for more detailed assertions in tests.
- Consider using a Map to store connections instead of a Set for more flexibility in testing.
- Implement updateAllProviders method to be consistent with updateProviders.
Example improvements:
private static class TestUseConnectionHolder extends ConnectionHolder {
private final Map<ProviderInfo, ClientTransport> connections = new ConcurrentHashMap<>();
final AtomicInteger calledCloseAllClientTransports = new AtomicInteger();
final AtomicInteger calledAddProvider = new AtomicInteger();
final AtomicInteger calledRemoveProvider = new AtomicInteger();
// ... (existing constructor)
@Override
public ConcurrentMap<ProviderInfo, ClientTransport> getAvailableConnections() {
return new ConcurrentHashMap<>(connections);
}
@Override
public List<ProviderInfo> getAvailableProviders() {
return new ArrayList<>(connections.keySet());
}
@Override
public ClientTransport getAvailableClientTransport(ProviderInfo providerInfo) {
return connections.get(providerInfo);
}
@Override
public boolean isAvailableEmpty() {
return connections.isEmpty();
}
@Override
public Collection<ProviderInfo> currentProviderList() {
return new ArrayList<>(connections.keySet());
}
@Override
public void addProvider(ProviderGroup providerGroup) {
calledAddProvider.incrementAndGet();
updateProviders(providerGroup);
}
@Override
public void removeProvider(ProviderGroup providerGroup) {
calledRemoveProvider.incrementAndGet();
for (ProviderInfo info : providerGroup.getProviderInfos()) {
connections.remove(info);
}
}
@Override
public void updateProviders(ProviderGroup providerGroup) {
for (ProviderInfo info : providerGroup.getProviderInfos()) {
connections.put(info, null); // or mock ClientTransport if needed
}
}
@Override
public void updateAllProviders(List<ProviderGroup> providerGroups) {
connections.clear();
for (ProviderGroup group : providerGroups) {
updateProviders(group);
}
}
// ... (other methods remain the same)
}
These enhancements will make the TestUseConnectionHolder more versatile for current and future tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation:
fix connection leakage.
Modification:
Describe the idea and modifications you've done.
Result:
Fixes #1442 .
Summary by CodeRabbit
New Features
Bug Fixes
Tests