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

Optimize performance of '/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces' interface queries #4473

Merged
merged 53 commits into from
Jul 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
fe75b4d
add tech-support-qq-4.png
klboke May 16, 2019
99bf97a
Update README.md
klboke May 16, 2019
1579f41
Merge remote-tracking branch 'upstream/master'
klboke Nov 11, 2020
9f65eed
Merge remote-tracking branch 'upstream/master'
klboke Dec 9, 2020
d7d3fd9
Enhance the user experience in the scenario of submitting duplicate keys
klboke Dec 11, 2020
7329fab
Merge branch 'master' into master
nobodyiam Dec 12, 2020
5def448
Modify the key-value conflict exception prompt, adjust the code style
klboke Dec 12, 2020
6942564
Merge branch 'master' into master
nobodyiam Dec 12, 2020
0fc1f91
Merge remote-tracking branch 'upstream/master'
klboke Dec 19, 2020
61ad016
Merge remote-tracking branch 'origin/master'
klboke Dec 19, 2020
587ce33
rge remote-tracking branch 'upstream/master'
klboke Mar 1, 2021
9de6563
Merge remote-tracking branch 'upstream/master'
klboke Mar 8, 2021
9aab632
Merge remote-tracking branch 'upstream/master'
klboke Mar 25, 2021
050dd0a
Merge branch 'ctripcorp:master' into master
klboke May 10, 2021
5a64035
Merge branch 'master' of github.com:klboke/apollo
klboke May 10, 2021
0648fbd
Merge branch 'ctripcorp:master' into master
klboke May 19, 2021
35bd3a6
Merge branch 'master' of github.com:klboke/apollo
klboke May 19, 2021
bc8149b
Merge branch 'ctripcorp:master' into master
klboke Jun 8, 2021
e771bdd
Merge branch 'master' of github.com:klboke/apollo
klboke Jun 8, 2021
9a1ad83
Merge branch 'ctripcorp:master' into master
klboke Jun 25, 2021
3efef37
Merge branch 'master' of github.com:klboke/apollo
klboke Jun 25, 2021
1274281
Merge branch 'apolloconfig:master' into master
klboke Sep 1, 2021
a968932
Merge branch 'master' of github.com:klboke/apollo
klboke Sep 1, 2021
df8942c
Merge branch 'apolloconfig:master' into master
klboke Jan 7, 2022
f98175a
Merge branch 'master' of github.com:klboke/apollo
klboke Jan 7, 2022
710cdd6
Merge branch 'apolloconfig:master' into master
klboke Feb 18, 2022
3c162b6
Merge branch 'master' of github.com:klboke/apollo
klboke Feb 18, 2022
0101a3f
Merge branch 'apolloconfig:master' into master
klboke Jun 17, 2022
cc6f568
Merge branch 'master' of github.com:klboke/apollo
klboke Jun 17, 2022
21dd30c
Merge branch 'apolloconfig:master' into master
klboke Jun 21, 2022
c1eeb41
Merge branch 'master' of github.com:klboke/apollo
klboke Jun 21, 2022
e85d645
Merge branch 'apolloconfig:master' into master
klboke Jun 27, 2022
8b0056d
Merge branch 'master' of github.com:klboke/apollo
klboke Jun 27, 2022
347e07b
Merge branch 'apolloconfig:master' into master
klboke Jul 1, 2022
d8eb437
Merge branch 'master' of github.com:klboke/apollo
klboke Jul 1, 2022
19bae52
Merge branch 'apolloconfig:master' into master
klboke Jul 4, 2022
7e40fa1
Merge branch 'master' of github.com:klboke/apollo
klboke Jul 4, 2022
fbbb02d
Merge branch 'apolloconfig:master' into master
klboke Jul 6, 2022
19cee24
Merge branch 'master' of github.com:klboke/apollo
klboke Jul 6, 2022
91ba3b7
Merge branch 'apolloconfig:master' into master
klboke Jul 8, 2022
94ad542
Merge branch 'master' of github.com:klboke/apollo
klboke Jul 8, 2022
64e1b0b
Merge branch 'apolloconfig:master' into master
klboke Jul 19, 2022
36ce5e2
Merge branch 'master' of github.com:klboke/apollo
klboke Jul 19, 2022
14da645
pref(apollo-portal): Optimize performance of '/apps/{appId}/envs/{env…
klboke Jul 20, 2022
e3eccdb
doc(CHANGES.md): update CHANGES.md
klboke Jul 20, 2022
1305cbf
Merge branch 'master' into namespaces
klboke Jul 21, 2022
f5d8522
Merge branch 'master' into namespaces
klboke Jul 21, 2022
2a466f5
fix(apollo-portal/NamespaceService): Fix Expected exceptions not thro…
klboke Jul 21, 2022
f79d1cd
refactor(apollo-portal/NamespaceService): Enhanced exception log info…
klboke Jul 22, 2022
1cb6f29
fix(apollo-portal/NamespaceService): Repair test cases
klboke Jul 22, 2022
ad42aef
Update apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/…
klboke Jul 24, 2022
e67d2a5
Merge branch 'master' into namespaces
klboke Jul 24, 2022
c9de54d
refactor(apollo-portal/NamespaceService): Replace CopyOnWriteArrayLis…
klboke Jul 24, 2022
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
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ Apollo 2.1.0
* [Allow users to associate multiple public namespaces at a time](https://github.com/apolloconfig/apollo/pull/4437)
* [Move apollo-demo, scripts/docker-quick-start and scripts/apollo-on-kubernetes out of main repository](https://github.com/apolloconfig/apollo/pull/4440)
* [Add search key when comparing Configuration items](https://github.com/apolloconfig/apollo/pull/4459)
* [Optimize performance of '/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces' interface queries](https://github.com/apolloconfig/apollo/pull/4473)
* [Add a new API to load items with pagination](https://github.com/apolloconfig/apollo/pull/4468)
* [fix(#4474):'openjdk:8-jre-alpine' potentially causing wrong number of cpu cores](https://github.com/apolloconfig/apollo/pull/4475)

------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo/milestone/11?closed=1)
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.ctrip.framework.apollo.common.exception.BadRequestException;
import com.ctrip.framework.apollo.common.utils.BeanUtils;
import com.ctrip.framework.apollo.core.enums.ConfigFileFormat;
import com.ctrip.framework.apollo.core.utils.ApolloThreadFactory;
import com.ctrip.framework.apollo.core.utils.StringUtils;
import com.ctrip.framework.apollo.portal.api.AdminServiceAPI;
import com.ctrip.framework.apollo.portal.component.PortalSettings;
Expand All @@ -41,11 +42,15 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gson.Gson;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.annotation.Lazy;
Expand All @@ -58,6 +63,9 @@ public class NamespaceService {

private static final Logger LOGGER = LoggerFactory.getLogger(NamespaceService.class);
private static final Gson GSON = new Gson();
private static final ExecutorService executorService = Executors.newFixedThreadPool(
Runtime.getRuntime().availableProcessors() * 2
, ApolloThreadFactory.create("NamespaceService", true));

private final PortalConfig portalConfig;
private final PortalSettings portalSettings;
Expand Down Expand Up @@ -165,20 +173,35 @@ public List<NamespaceBO> findNamespaceBOs(String appId, Env env, String clusterN
throw new BadRequestException("namespaces not exist");
}

List<NamespaceBO> namespaceBOs = new LinkedList<>();
List<NamespaceBO> namespaceBOs = Collections.synchronizedList(new LinkedList<>());
List<String> exceptionNamespaces = Collections.synchronizedList(new LinkedList<>());
CountDownLatch latch = new CountDownLatch(namespaces.size());
for (NamespaceDTO namespace : namespaces) {
executorService.submit(() -> {
NamespaceBO namespaceBO;
try {
namespaceBO = transformNamespace2BO(env, namespace);
namespaceBOs.add(namespaceBO);
} catch (Exception e) {
LOGGER.error("parse namespace error. app id:{}, env:{}, clusterName:{}, namespace:{}",
appId, env, clusterName, namespace.getNamespaceName(), e);
exceptionNamespaces.add(namespace.getNamespaceName());
} finally {
latch.countDown();
}
});

NamespaceBO namespaceBO;
try {
namespaceBO = transformNamespace2BO(env, namespace);
namespaceBOs.add(namespaceBO);
} catch (Exception e) {
LOGGER.error("parse namespace error. app id:{}, env:{}, clusterName:{}, namespace:{}",
appId, env, clusterName, namespace.getNamespaceName(), e);
throw e;
}
}
try {
latch.await();
klboke marked this conversation as resolved.
Show resolved Hide resolved
} catch (InterruptedException e) {
//ignore
}

if(namespaceBOs.size() != namespaces.size()){
throw new RuntimeException(String
.format("Parse namespaces error, expected: %s, but actual: %s, cannot get those namespaces: %s", namespaces.size(), namespaceBOs.size(), exceptionNamespaces));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic will throw the actual exception so that the user knows what's wrong. Is it better to preserve that logic? For example, we could use future.get to retrieve the exception occurred.

    List<NamespaceBO> namespaceBOs = Collections.synchronizedList(new LinkedList<>());
    List<Future<?>> futures = new LinkedList<>();
    for (NamespaceDTO namespace : namespaces) {
      futures.add(executorService.submit(() -> {
        NamespaceBO namespaceBO;
        try {
          namespaceBO = transformNamespace2BO(env, namespace);
          namespaceBOs.add(namespaceBO);
        } catch (Exception e) {
          LOGGER.error("parse namespace error. app id:{}, env:{}, clusterName:{}, namespace:{}",
              appId, env, clusterName, namespace.getNamespaceName(), e);
          throw e;
        }
      }));
    }

    futures.forEach(future -> {
      try {
        future.get();
      } catch (ExecutionException | InterruptedException e) {
        Throwable exception = e instanceof ExecutionException ? e.getCause() : e;
        if (exception instanceof RuntimeException) {
          throw (RuntimeException) exception;
        } else {
          throw new RuntimeException(exception);
        }
      }
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOGGER.error("parse namespace error. app id:{}, env:{}, clusterName:{}, namespace:{}",
              appId, env, clusterName, namespace.getNamespaceName(), e);

The original exception stack information will be recorded here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand they are recorded in the server logs. What I propose is to throw the detailed exception message to the UI page so that the operating users could understand what's wrong, which is the original behavior.
The current error message Parse namespaces error, expected: %s, but actual: %s, cannot get those namespaces: %s could show the operating users some error details, but I have a concern that it might not be very clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably see what you mean, and the main points I took into account when making the changes here were.

  • I am not very clear about the internal logic will report what possible exceptions, but when I simulated an exception, as a direct user it is difficult to do anything based on this exception, may be my simulation and the real difference
  • the original will only encounter a Namespace problem to report an exception, now all the problems will be reported at once, will it be better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very clear about the internal logic will report what possible exceptions, but when I simulated an exception, as a direct user it is difficult to do anything based on this exception, may be my simulation and the real difference

The exceptions would be either timeout exceptions occurred in the portal side, or database access exceptions occurred in the admin service side. I suppose there is not much to do for a direct user, except that the user could report that to the admin users.
So my original idea was that we keep the logic the same as before - throw the actual exception and display it in the UI side, but I think the current logic is also ok.

the original will only encounter a Namespace problem to report an exception, now all the problems will be reported at once, will it be better?

That I suppose is the same as the user can't really handle the errors, no matter it contains one error or multiple errors :-)

}
return namespaceBOs;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.Collections;
import java.util.List;

import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -127,6 +128,14 @@ public void testFindNamespace() {
assertEquals(testClusterName, namespaceVO.getBaseInfo().getClusterName());
assertEquals(testNamespaceName, namespaceVO.getBaseInfo().getNamespaceName());

ReleaseDTO errorRelease = new ReleaseDTO();
someRelease.setConfigurations("\"a\":\"123\",\"b\":\"123\"");
when(releaseService.loadLatestRelease(testAppId, Env.DEV, testClusterName, "hermes")).thenReturn(errorRelease);
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(()-> namespaceService.findNamespaceBOs(testAppId, Env.DEV, testClusterName))
.withMessageContaining("hermes", testNamespaceName)
.withMessageStartingWith("Parse namespaces error, expected: 2, but actual: 0, cannot get those namespaces: ");

}

@Test
Expand Down