Convert DASH meter bucket object to table entry#2056
Convert DASH meter bucket object to table entry#2056prsunny merged 1 commit intoopencomputeproject:masterfrom
Conversation
Bring in SAI API changes from this DASH PR sonic-net/DASH#605 Signed-off-by: mukeshmv <mukesh@pensando.io>
|
Could someone add @prsunny and @marian-pritsak to the Reviewers here? |
| if (not $fnparams =~ /^\w+_id number_of_counters counter_ids( (mode )?counters)?$/) | ||
| { | ||
| LogWarning "invalid stat function $fname params names: $fnparams"; | ||
| if (not $fnparams =~ /^\w+_entry number_of_counters counter_ids( (mode )?counters)?$/) |
There was a problem hiding this comment.
@mukeshmv as I mentioned in the DASH PR, the right way to do this is to check in this file only and bring it to DASH. Now, the DASH SAI is diverged from OCP SAI, because it is not superset anymore. Any future DASH update is going to have a chance to break this feature, as the generated API is still going to use the old SAI object format for metering bucket. It is not good to leave DASH in this state. Can you help bring this change back to DASH?
Hi @prsunny , do you mind to help track this? I am OOF and My internet access is horrible, so I won't be able to do this myself.
There was a problem hiding this comment.
The style.pm change is not sufficient to get the Meter bucket API change in since the API change is not backward compatible. Hence we had to resort to pushing this directly to SAI. I have a new PR open in Dash to pull this in along with your original commit that should resolve this.
There was a problem hiding this comment.
This is not backward compatible ! Remove !!
There was a problem hiding this comment.
@kcudnik, could you elaborate why you think this style change is not backward compatible ? The change actually relaxes the check by allowing another variation in addition to the existing stats API. I don't see how this breaks backward compatibility.
There was a problem hiding this comment.
Change is equivalent to adding an "or" to existing check - allowing an extra pattern.
- if (not $fnparams =~ /^\w+_id number_of_counters counter_ids( (mode )?counters)?$/)
+ if (not ($fnparams =~ /^\w+_id number_of_counters counter_ids( (mode )?counters)?$/ or
+ $fnparams =~ /^\w+_entry number_of_counters counter_ids( (mode )?counters)?$/))
{
LogWarning "invalid stat function $fname params names: $fnparams";
}
If you want I can change it to this for better readability.
|
This style change is not backward compatible |
|
Please. Remove style change |
|
@prsunny don't merge changes like this !! |
|
@mukeshmv , lets remove the style change and only have the dash changes in these PRs. all other changes other than dash should go as seperate PR/approval |
@kcudnik , can you check Mukesh's comment? If required, we can revert this PR. |
|
Why added API is not following proposed standard ? |
@kcudnik, the reason is explained in @r12f 's Dash PR sonic-net/DASH#605 |
|
@mukeshmv, @r12f, @prsunny, @kcudnik the style change relaxes stats API check. While compiling sonic-sairedis with this PR, it will fail. SaiInterface (meta/SaiInterface.h) still does not support stats API for sai_xxx_entry. |
) Bring in SAI API changes from this DASH PR sonic-net/DASH#605 Signed-off-by: siqbal1986 <shahzad.iqbal@microsoft.com>
) Bring in SAI API changes from this DASH PR sonic-net/DASH#605
7 participants
Bring in SAI API changes from this DASH PR
sonic-net/DASH#605