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

Add demonstration generated code for ordered maps in ygot #819

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Apr 20, 2023

Once this is approved development for code generation can begin.

@wenovus wenovus requested review from DanG100 and bormanp April 20, 2023 19:51
@github-actions
Copy link

github-actions bot commented Apr 20, 2023

Coverage Status

Coverage: 90.208% (+0.004%) from 90.204% when pulling ea9643f on demo-ordered-map into 4cd47b4 on master.

gogen/internal/gotypes/ordered_map.go Outdated Show resolved Hide resolved
}

// Update updates a current policy statement, returning an error if the statement
// doesn't exist or if the key is unspecified.
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think Update and AppendNew would be that useful, what use cases did you have in mind for them.

I'd rather have GetOrAppend(key) *RoutingPolicyDefinition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added Update as a basic dictionary operation. I don't have a specific use case in mind. I think it's ok to remove it if we want to keep to specific use cases.

AppendNew was added because Scott mentioned the current New will be useful alongside Append. This would be the equivalent for ordered maps.

As for GetOrAppend, this was in the draft I proposed originally since it follows existing conventions with unordered maps. I received feedback that this might be confusing because they want to know whether it was a Get or Append, since with ordered maps each element in general (e.g. BGP policies) cares about what other elements already exist, whereas in unordered maps each element often does not care about the existence of other elements (e.g. interfaces). So splitting it into Get and AppendNew helps the user write code that's less ambiguous. The stakeholders mentioned New and Append as the primary functionalities that they would use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try to get a code review from the users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I got feedback that Append is probably all they will need. For AppendNew I think given this API exists in the existing API method set, that it does not subtract value to have. I've removed Update though since I agree it might be a stretch for people to use it. PTAL

Copy link
Contributor

@DanG100 DanG100 Apr 21, 2023

Choose a reason for hiding this comment

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

here's my thoughts.

  1. GetOrCreate is by far the most commonly used function (at least in featureprofiles). Because it doesn't return an error, it is really convenient to use.
  2. Append: is very rarely used, though it's a still valuable and should exist
  3. AppendNew: I don't see this in the existing API set, and personally don't see when i'd use it.

however, i'm fine with the API as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with 1&2 from a current-API perspective, the current PR reflects iteration after feedback. Ordered-maps are different than unordered-maps and IMO this method set has greater usability than simply copying what the existing unordered-map API does.

For 3 AppendNew corresponds to New for unordered maps.

Copy link

@bormanp bormanp left a comment

Choose a reason for hiding this comment

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

Sorry, I just realized my comments were still pending :-(

gogen/internal/gotypes/ordered_map.go Show resolved Hide resolved
type RoutingPolicy_PolicyDefinition_Statement_Map struct {
// TODO: Add a mutex here and add race tests after implementing
// ygot.Equal and evaluating the thread-safety of ygot.
//mu sync.RWmutex
Copy link

Choose a reason for hiding this comment

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

You should add locking code now around any fields that could be modified while other code is reading/modifying them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can add now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I will hold off pending our discussion below, I'm still not convinced it's the right time to add this since the rest of ygot doesn't deal with thread-safety. If we do want thread-safety I think we would want another suite of tests. I also think this can be added in a separate PR.


// Keys returns a copy of the list's keys.
func (o *RoutingPolicy_PolicyDefinition_Statement_Map) Keys() []string {
return append([]string{}, o.keys...)
Copy link

Choose a reason for hiding this comment

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

All of these functions should grab a mutex for the duration of their access to the fields in the struct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add it. Although the reason why I don't think it's important to add this now is because ygot-generated code is not currently thread-safe. There are instances of slice types, and slices by themselves are not thread-safe -- they would have to be encapsulated just like this type for them to be so. The other pointer types are technically ok because they're benign data races, and perhaps in Go these are truly benign, although I'm not 100% sure.

}

// Len returns a size of RoutingPolicy_PolicyDefinition_Statement_Map
func (o *RoutingPolicy_PolicyDefinition_Statement_Map) Len() int {
Copy link

Choose a reason for hiding this comment

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

Should there be the typical race condition warning that the return value may become incorrect immediately after being returned? Is this more a desire to know if it is empty or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sigh, I think similar to the above, ygot was not designed to be thread-safe, and so for the current map and slice types users would be calling len() just like this function does. IMO it doesn't seem very useful to the user if one very-rare type in the GoStruct is thread-safe while everything else needs a lock anyways. That's why I'm not sure about the value of making this thread-safe at the current time.

gogen/internal/gotypes/ordered_map.go Outdated Show resolved Hide resolved
gogen/internal/gotypes/ordered_map.go Show resolved Hide resolved
@wenovus wenovus requested review from DanG100 and bormanp April 21, 2023 21:46
@wenovus wenovus merged commit 37a3f75 into master Apr 25, 2023
@wenovus wenovus deleted the demo-ordered-map branch April 25, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants