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

Send GoalStateV2 from DPM, NCM and ACA #704

Merged
merged 60 commits into from
Nov 29, 2021
Merged

Conversation

DavidLiu506
Copy link
Contributor

1 Merge GoalState in DPM and send to NCM, wait for NCM response.
2 Send GoalState from NCM to ACA and wait for all ACA response.
3 Improve NCM performance.
4 Use linkerd load balance grpc request for NCM.

@xieus xieus added the enhancement New feature or request label Nov 24, 2021
@xieus xieus added this to the Version 1.0.2021.12.30 milestone Nov 24, 2021
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.

@DavidLiu506 Please take a look at the following few comments. Thanks.

Comment on lines +92 to +94
- port: 50001
protocol: TCP
targetPort: 50001
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this change block port 9014 for REST APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for grpc configure, we can add another service for restful.

schema/proto3/common.proto Outdated Show resolved Hide resolved
Comment on lines +74 to +75
- containerPort: 50001
protocol: TCP
Copy link
Contributor

Choose a reason for hiding this comment

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

@DavidLiu506 Why we need to change 8080 to 50001 here? Is this for grpc connection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is for grpc connection.

Comment on lines +92 to +94
- port: 50001
protocol: TCP
targetPort: 50001
Copy link
Contributor

Choose a reason for hiding this comment

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

@DavidLiu506 Do we need to include port spec for restful here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can add restful configure after restful api fix.

Comment on lines +76 to +79
if (!finishLatch.await(1, TimeUnit.MINUTES)) {
LOG.warn("Send goal states can not finish within 1 minutes");
return Arrays.asList("Send goal states can not finish within 1 minutes");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@DavidLiu506 Do we know which goalstate or which ncm cannot sent to? If we do, can we put the id or uri in the log and return results?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, can we put 1 mins as a variable in application.properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NCM managed by linked, there only one goal state v2 send to NCM.

Copy link
Contributor Author

@DavidLiu506 DavidLiu506 Nov 25, 2021

Choose a reason for hiding this comment

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

Talked with James, NCM already have ACA response information. DPM don't need to know it.

@xieus xieus self-requested a review November 29, 2021 18:05
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.

@cj-chung cj-chung self-requested a review November 29, 2021 19:55
Copy link
Contributor

@cj-chung cj-chung 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 merged commit a843622 into futurewei-cloud:master Nov 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants