Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better if we do not change it by default, it is better to first introduce this as option and then wait till it becomes stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lguohan
Many thanks for the review.
I added a compile-time option
ENABLE_DPLANE_FPM_SONIC
.When this option is set, SONiC uses the new FPM module.
When this option is not set, SONiC uses the old FPM module and behaves exactly as it does today.
By default, this option is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am fine that we build them into image by default, but do not enable it by default. if you can do it at runtime, it will be easier for you, and i am fine with that.
does this dplane_fpm_sonic support all features as dplane_fpm_nl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lguohan
Yes, the
dplane_fpm_sonic
supports all features asdplane_fpm_nl
. It is a copy of the olddplane_fpm_nl
.Indeed, it is easier to compile this by default and keep the old
dplane_fpm_nl
running by default.I reverted the compile option and changed the supervisord config file to keep
dplane_fpm_nl
as default.