search

p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
140 stars 86 forks source link

Support for initial entries #432

Closed jafingerhut closed 11 months ago

jafingerhut commented 1 year ago

Fixes https://github.com/p4lang/p4runtime/issues/426

jafingerhut commented 1 year ago

As of commit 2 of this PR, this is a first draft of a proposal for addressing issue https://github.com/p4lang/p4runtime/issues/426

There are still several embedded questions marked TODO at this point. Please comment with suggested answers to those questions if you have any.

jafingerhut commented 1 year ago

Jonathan DiLorenzo: Should we have tables with const entries have has_initial_entries AND is_const_table both true?

Andy: I will make this change in another commit on this PR and let people know it is ready for review.

jafingerhut commented 1 year ago

I have updated this PR to follow the suggestion made during the 2023-Jul-14 API work group meeting, so that tables declared in source code with const entries continue to have is_const_table true as before, but they also have the new flag has_initial_entries true.

The new style of tables that the language spec now supports that can have entries without const before entries, will only have has_initial_entries true, but is_const_table will be false.

jafingerhut commented 1 year ago

@smolkaj @jonathan-dilorenzo @chrispsommers I have made 7 commits to this PR since the 2023-Jul-14 API work group meeting, primarily due to comments made during the meeting, and from Steffen's careful review after the meeting, and I think it is the better for it.

I have no current plans to change it any more, until and unless there is additional review feedback that warrants additional changes.

smolkaj commented 12 months ago

This LGTM! There seem to be 3 remaining threads that would be nice to resolve, 2 of them being nits. Other than that, from my side this is ready to be merged.

jafingerhut commented 12 months ago

LGTM modulo the remaining open comment in the appendix regarding the protobuf schema.

With commit 22 on this PR, I have attempted to clarify the description of the protobuf schema of an entries file.

smolkaj commented 12 months ago

Looks great to me.

jafingerhut commented 12 months ago

@saynb @jamescchoi This has been through careful review by several others already, so hopefully should also meet with your approval, but in case you want to take a look, this is something we are going to be asked to implement by some folks, so good to be aware of the details.

jafingerhut commented 11 months ago

@smolkaj @chrispsommers Happy to discuss this at the next P4 API work group meeting on 2023-Aug-11, in hopes of seeing if it has the right approvals for merging.

smolkaj commented 11 months ago

We have 4 approvals, so I think we are good to go. Let's wait until the meeting before we hit merge, just in case.