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

[syncd] Add workaround for port error status notification #1430

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Oct 10, 2024

Adding new field in port status notification is breaking
change and can cause some issues when getting notification
from older SAI library, this will add workaround to handle
that case

Add changes to handle new data types
Adding new field in port status notification is breaking
change and can cause some issues when getting notification
from older SAI library, this will add workaround to handle
that case
@kcudnik kcudnik marked this pull request as draft October 10, 2024 13:50
@kcudnik
Copy link
Collaborator Author

kcudnik commented Oct 10, 2024

I'm just creating this PR to see what i miss from build, i will later split this to 2 PR's as advancing SAI submodule and then commiting actual workaround

@@ -1300,6 +1354,52 @@ sai_status_t VendorSai::bulkRemove(
object_statuses);
}

sai_status_t VendorSai::bulkRemove(
Copy link

Choose a reason for hiding this comment

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

@kcudnik I see this change is not relevant as per the PR title. Can this be kept outside this PR and added in another PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

read this comment: #1430 (comment), this is 1st pr: #1431

@@ -998,6 +998,60 @@ sai_status_t VendorSai::bulkCreate(
object_statuses);
}

sai_status_t VendorSai::bulkCreate(
Copy link

Choose a reason for hiding this comment

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

@kcudnik this change be added as another PR as this is NOT relevant as per the PR title

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

read this comment: #1430 (comment), this is 1st pr: #1431

@prgeor
Copy link

prgeor commented Oct 12, 2024

@kcudnik could you please check the build failure.

@kcudnik
Copy link
Collaborator Author

kcudnik commented Oct 13, 2024

Yes, already fixed in latest pr #1431 but swss repo tests are failing because of sai backward compatibility break, I already fixed that and posted pr on swss repo pleased follow thread on #1431

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.

2 participants