Skip to content
This repository was archived by the owner on Mar 31, 2023. It is now read-only.

Conversation

@VanderChen
Copy link
Contributor

Implement #490 as Port Manager style and there still be a problem to solve.

@xieus xieus requested a review from chenpiaoping January 7, 2021 01:31
@xieus xieus added the feature feature development label Jan 7, 2021
@xieus xieus added this to the Version 1.0.2021.02.28 milestone Jan 7, 2021

@Override
public void addNodeInfo(NodeInfo nodeInfo) throws Exception {
nodeInfoCache.addNodeInfo(nodeInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to cache all the fields of NodeInfo here?

nodeInfo = nodeInfoCache.get(nodeIp);
assert nodeInfo != null;
} catch (Exception e) {
NodeInfo newNodeInfo = nodeManagerRestClient.getNodeInfo(nodeIp).getNodeInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

newNodeInfo may be null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A NodeInfoNotFound Exception has been added here.

}

@DurationStatistics
public synchronized void addNodeInfo(NodeInfo nodeInfo) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need synchronized here?

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 have remove it.

}

for (String hostIp : hostIps) {
String groupTopic = localCache.getNodeInfo(hostIp).getGroupTopic();
Copy link
Contributor

Choose a reason for hiding this comment

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

localCache.getNodeInfo(hostIp) may rerturn null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A Exception has been added here.

throw new ParameterNullOrEmptyException("");
}
try {
nodeService.createNodeInfo(nodeInfoJson);
Copy link
Contributor

Choose a reason for hiding this comment

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

updateNodeInfo()?

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 have fixed this bug.

throw new ParameterNullOrEmptyException("");
}
try {
nodeService.createNodeInfo(nodeInfoJson);
Copy link
Contributor

Choose a reason for hiding this comment

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

deleteNodeInfo()?

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 have fixed this bug.

public void createNodeInfoBulk(BulkNodeInfoJson bulkNodeInfoJson) throws Exception {
List<NodeInfo> nodeInfos = bulkNodeInfoJson.getNodeInfos();
for (NodeInfo nodeInfo : nodeInfos) {
localCache.addNodeInfo(nodeInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

localCache should support batch operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addNodeInfoBulk method has been added to LocalCache

@DurationStatistics
public void createNodeInfo(@RequestBody NodeInfoJson nodeInfoJson) throws Exception {
if (nodeInfoJson == null) {
throw new ParameterNullOrEmptyException("");
Copy link
Contributor

Choose a reason for hiding this comment

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

More checks need to be done here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More checks have been added here.

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #532 (0e45596) into master (17226df) will decrease coverage by 0.42%.
The diff coverage is 12.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #532      +/-   ##
============================================
- Coverage     35.20%   34.78%   -0.43%     
- Complexity     1234     1241       +7     
============================================
  Files           491      502      +11     
  Lines         12157    12373     +216     
  Branches       1545     1557      +12     
============================================
+ Hits           4280     4304      +24     
- Misses         7303     7489     +186     
- Partials        574      580       +6     
Impacted Files Coverage Δ Complexity Δ
...rewei/alcor/nodemanager/processor/NodeContext.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...wei/alcor/nodemanager/request/AbstractRequest.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...nodemanager/request/BulkCreateNodeInfoRequest.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...cor/nodemanager/request/CreateNodeInfoRequest.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...cor/nodemanager/request/DeleteNodeInfoRequest.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...ewei/alcor/nodemanager/request/RequestManager.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...cor/nodemanager/request/UpdateNodeInfoRequest.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../nodemanager/service/implement/NodeFileLoader.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...nodemanager/service/implement/NodeServiceImpl.java 1.35% <0.00%> (-0.44%) 2.00 <0.00> (ø)
...i/alcor/nodemanager/utils/NodeManagerConstant.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17226df...0e45596. Read the comment docs.

@chenpiaoping
Copy link
Contributor

LGTM

@VanderChen
Copy link
Contributor Author

@xieus Excuse me. I wonder whether this PR is ready for merge

Copy link
Contributor

@xieus xieus left a comment

Choose a reason for hiding this comment

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

LGTM.

@xieus xieus changed the title Suppot Node-MQ Topic Mapping [Node Mgr] Suppot Node-MQ Topic Mapping Feb 17, 2021
@xieus xieus merged commit 91d6298 into futurewei-cloud:master Feb 17, 2021
@VanderChen VanderChen deleted the mq-mapping branch October 18, 2021 02:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature feature development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants