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.

Sign up for GitHub

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

Jump to bottom

give out-of-package access to location fileds. this is needed for REST calls as part of the CRV #39

Closed
wants to merge 1 commit into from
Closed

give out-of-package access to location fileds. this is needed for REST calls as part of the CRV #39

wants to merge 1 commit into from

Conversation

yb01
Copy link
Collaborator

@yb01 yb01 commented Jun 23, 2022 •
edited
Loading

the region and partition of the Location type is needed to be in REST req and resp for the CRVs. the fileds is currently accessible only for in-package functions, which causes the CRV is not transmitted in REST calls. below is an example with the region service simulator and aggregators in the POSTCRV calls.

the sim.log is from the simulator where the CRVs are posted and the t.log is the aggregrator which pushes the CRVs. you can see 10 in the map with the values and only one shows in the simulator with fake values.

with this PR, all 10 shows in both sides.

yunwens-mbp:resource-management yunwenbai$ grep -i debug sim.log
I0623 16:09:05.093987   69241 regionNodeEvents.go:110] debug: CRV is map[{0 0}:5000]
I0623 16:09:05.094020   69241 regionNodeEvents.go:113] debug: RV in map, key: Beijing-RP1, value: 5000
I0623 16:09:21.871014   69241 regionNodeEvents.go:110] debug: CRV is map[{0 0}:5000]
I0623 16:09:21.871035   69241 regionNodeEvents.go:113] debug: RV in map, key: Beijing-RP1, value: 5000
yunwens-mbp:resource-management yunwenbai$ grep -i debug t.log
I0623 16:09:05.093517   69251 aggregator.go:183] debug: CRV is map[{8 0}:5000 {8 1}:5000 {8 2}:5000 {8 3}:5000 {8 4}:5000 {8 5}:5000 {8 6}:5000 {8 7}:5000 {8 8}:5000 {8 9}:5000]
I0623 16:09:05.093573   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP8, value: 5000
I0623 16:09:05.093590   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP10, value: 5000
I0623 16:09:05.093599   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP1, value: 5000
I0623 16:09:05.093607   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP2, value: 5000
I0623 16:09:05.093616   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP3, value: 5000
I0623 16:09:05.093624   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP4, value: 5000
I0623 16:09:05.093633   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP7, value: 5000
I0623 16:09:05.093642   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP5, value: 5000
I0623 16:09:05.093651   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP6, value: 5000
I0623 16:09:05.093659   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP9, value: 5000
I0623 16:09:21.870676   69251 aggregator.go:183] debug: CRV is map[{8 0}:5000 {8 1}:5000 {8 2}:5000 {8 3}:5000 {8 4}:5000 {8 5}:5000 {8 6}:5000 {8 7}:5000 {8 8}:5000 {8 9}:5000]
I0623 16:09:21.870712   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP3, value: 5000
I0623 16:09:21.870722   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP5, value: 5000
I0623 16:09:21.870728   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP8, value: 5000
I0623 16:09:21.870733   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP9, value: 5000
I0623 16:09:21.870739   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP10, value: 5000
I0623 16:09:21.870744   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP2, value: 5000
I0623 16:09:21.870750   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP4, value: 5000
I0623 16:09:21.870755   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP6, value: 5000
I0623 16:09:21.870761   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP7, value: 5000
I0623 16:09:21.870766   69251 aggregator.go:186] debug: RV in map, key: Reserved5-RP1, value: 5000

@yb01 yb01 requested review from Sindica and q131172019 and removed request for Sindica June 23, 2022 23:21
q131172019
q131172019 reviewed Jun 24, 2022
View reviewed changes
Copy link
Collaborator

@q131172019 q131172019 left a comment

Choose a reason for hiding this comment

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

If Ying and Sonya have no response by this Friday afternoon, we can consider to approve this PR and merge into main branch first. If there are incoming issues, we can make further changes.

Sindica
Sindica reviewed Jun 24, 2022
View reviewed changes
resource-management/pkg/common-lib/types/location/location.go
@@ -8,14 +8,14 @@ import (
const RingRange = float64(360)

type Location struct {
region Region
partition ResourcePartition
Region Region
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned before, Location is a critical data structure that is used as the base of hash ring. It is absolutely not allowed to change its value. Therefore, it should not use uppercase field names. If you need to use it, please make your own data structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we will create a new structure for transit and copy over the CRV info in this case.

Sindica
Sindica reviewed Jun 24, 2022
View reviewed changes
resource-management/pkg/common-lib/types/location/location.go
@@ -247,11 +247,11 @@ func GetRPsForRegion(region Region) []ResourcePartition {
}

func (loc *Location) GetRegion() Region {
return loc.region
return loc.Region
Copy link
Collaborator

Choose a reason for hiding this comment

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

With getter, you don't need to make field name uppercase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we will create a new structure for CRV in transit for now.

@yb01
Copy link
Collaborator Author

yb01 commented Jun 27, 2022

close this PR and we will get new data struct for CRV in transit.

@q131172019 q131172019 mentioned this pull request Jun 28, 2022
Closed
@yb01 yb01 closed this Jun 28, 2022
@yb01 yb01 reopened this Jun 28, 2022
@yb01
Copy link
Collaborator Author

yb01 commented Jul 1, 2022

close this PR and to preserve the commit for wordaround of the serialization issue in test. new PR #67 was created with the fix as proposed.

@yb01 yb01 closed this Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@q131172019 q131172019 q131172019 left review comments

@Sindica Sindica Sindica left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yb01 @Sindica @q131172019