Skip to content
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

Merged
merged 1 commit into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -210,19 +210,22 @@ public void updateProviders(ProviderGroup providerGroup) {
checkProviderInfo(providerGroup);
ProviderGroup oldProviderGroup = addressHolder.getProviderGroup(providerGroup.getName());
if (ProviderHelper.isEmpty(providerGroup)) {
boolean previouslyEmpty = ProviderHelper.isEmpty(oldProviderGroup);
addressHolder.updateProviders(providerGroup);
if (!ProviderHelper.isEmpty(oldProviderGroup)) {
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 {
addressHolder.updateProviders(providerGroup);
connectionHolder.updateProviders(providerGroup);
}
if (EventBus.isEnable(ProviderInfoUpdateEvent.class)) {
// see: https://github.com/sofastack/sofa-rpc/issues/1442
// there is a swallow copy problem which makes the oldProviderGroup same as providerGroup
ProviderInfoUpdateEvent event = new ProviderInfoUpdateEvent(consumerConfig, oldProviderGroup, providerGroup);
EventBus.post(event);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.alipay.sofa.rpc.client;

import com.alipay.sofa.rpc.bootstrap.ConsumerBootstrap;
import com.alipay.sofa.rpc.config.ConsumerConfig;
import com.alipay.sofa.rpc.core.exception.SofaRpcException;
import com.alipay.sofa.rpc.core.request.SofaRequest;
import com.alipay.sofa.rpc.core.response.SofaResponse;
import com.alipay.sofa.rpc.transport.ClientTransport;
import org.junit.Assert;
import org.junit.Test;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicInteger;

/**
*
* @author junyuan
* @version ClusterProviderUpdateTest.java, v 0.1 2024-10-11 11:04 junyuan Exp $
*/
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);
}
Comment on lines +41 to +86
Copy link
Contributor

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());
}
Comment on lines +88 to +104
Copy link
Contributor

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:

  1. Test with an empty provider list in addition to null.
  2. Verify the exact contents of the provider list after update, not just its size.
  3. Add a test case for updating with a different set of providers to ensure proper addition and removal.
  4. 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() {

}
}
Comment on lines +107 to +193
Copy link
Contributor

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:

  1. Implement the unimplemented methods with basic functionality or throw UnsupportedOperationException with a clear message.
  2. Add more tracking variables (similar to calledCloseAllClientTransports) for other important methods to allow for more detailed assertions in tests.
  3. Consider using a Map to store connections instead of a Set for more flexibility in testing.
  4. 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.

}
Loading