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

Sort Namespaces acquired by multiple threads #4500

Merged
merged 54 commits into from
Aug 8, 2022

Conversation

klboke
Copy link
Contributor

@klboke klboke commented Aug 3, 2022

What's the purpose of this PR

Sort Namespaces acquired by multiple threads due to #4473 causing the UI to display Namespaces out of order

klboke and others added 30 commits May 16, 2019 11:07
Comment on lines 208 to 210
return namespaceBOs.stream()
.sorted(Comparator.comparing(o -> o.getBaseInfo().getNamespaceName()))
.collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

How about sorting by namespace id?because the query logic here is to sort by namespace id.

Suggested change
return namespaceBOs.stream()
.sorted(Comparator.comparing(o -> o.getBaseInfo().getNamespaceName()))
.collect(Collectors.toList());
return namespaceBOs.stream()
.sorted(Comparator.comparing(o -> o.getBaseInfo().getId()))
.collect(Collectors.toList());

Copy link
Member

Choose a reason for hiding this comment

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

I think query by id is reasonable, which reflects the creation order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you use ID sorting, in different clusters, the order may be inconsistent, which is very unfriendly to users.

Copy link
Member

Choose a reason for hiding this comment

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

Previously, the order of the namespaceBOs was the same as the namespaces returned from namespaceAPI.findNamespaceByCluster(appId, env, clusterName). So I think it's better to keep the order whether we use parallel loading.

If you use ID sorting, in different clusters, the order may be inconsistent, which is very unfriendly to users.

The logic of namespaceAPI.findNamespaceByCluster(appId, env, clusterName) is to sort the namespace by id asc, which reflects the creation order. So I think it should be natural for the users to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for example:
image

  • The above is the namespace order of the hk cluster. When a new sg cluster is created, the time to generate the namespace is uncontrollable, which leads to the inconsistent order of the namespaces of the hk and sg clusters. When there are many namespaces, the problem caused by the inconsistency of the order will be more prominent.

image

  • However, on the UI interface, there is no place to display the creation time of the Namespace, so the user has no way of knowing why the Namespace is out of order.

  • The above is the reason why I think it is better to use Namespace Name sorting to keep the order of each cluster consistent

Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. I didn't consider the new cluster scenario.
If we sort the namespaces by name, is it possible that the application namespace is not displayed at the top? I have a concern that this might make the users confused...
If we want to make the namespaces order consistent among clusters, how about we adjust the create cluster logic? e.g. create private namespaces based on the appnamespace id order

@Transactional
public void instanceOfAppNamespaces(String appId, String clusterName, String createBy) {
List<AppNamespace> appNamespaces = appNamespaceService.findByAppId(appId);
for (AppNamespace appNamespace : appNamespaces) {
Namespace ns = new Namespace();
ns.setAppId(appId);
ns.setClusterName(clusterName);
ns.setNamespaceName(appNamespace.getName());
ns.setDataChangeCreatedBy(createBy);
ns.setDataChangeLastModifiedBy(createBy);
namespaceRepository.save(ns);
auditService.audit(Namespace.class.getSimpleName(), ns.getId(), Audit.OP.INSERT, createBy);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • It seems to make a little more sense to maintain the previous sorting logic and keep the order of Namespace creation consistent across clusters.
  • Before the main consideration has been how to solve the already existing disorder. Now it seems that direct ordering leads to more complicated problems, such as newly created Namespace may be ordered to an unknown place.

So, I agree that the ordering of ids should be maintained and has been changed, please review it again

@klboke klboke requested review from mghio and nobodyiam August 4, 2022 01:57
@codecov-commenter
Copy link

Codecov Report

Merging #4500 (a661b7c) into master (ec658b0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #4500      +/-   ##
============================================
+ Coverage     53.45%   53.46%   +0.01%     
- Complexity     2701     2703       +2     
============================================
  Files           490      490              
  Lines         15344    15346       +2     
  Branches       1596     1596              
============================================
+ Hits           8202     8205       +3     
  Misses         6584     6584              
+ Partials        558      557       -1     
Impacted Files Coverage Δ
...mework/apollo/biz/service/AppNamespaceService.java 32.46% <100.00%> (ø)
...mework/apollo/portal/service/NamespaceService.java 71.59% <100.00%> (+0.32%) ⬆️
...ervice/service/ReleaseMessageServiceWithCache.java 85.88% <0.00%> (+1.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit f5ce7aa into apolloconfig:master Aug 8, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
@klboke klboke deleted the kl-namesapce branch March 21, 2023 03:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants