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 main module for ovs forwarder #3

Merged
merged 8 commits into from
Aug 17, 2021

Conversation

pperiyasamy
Copy link
Member

@pperiyasamy pperiyasamy commented Jun 2, 2021

This is the main forwarder module hosting grpc endpoint for ovs forwarder which establishes vWire connection between service clients and endpoints.

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
TODO: replace local sdk-ovs with upstream sdk-ovs in go.mod

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@pperiyasamy pperiyasamy changed the title [WiP] add main module for ovs forwarder add main module for ovs forwarder Aug 5, 2021
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
main.go Outdated
// ********************************************************************************
// setup context to catch signals
// ********************************************************************************
ctx, cancel := getTermSigalContext()
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend using signal.NotifyContext

main.go Outdated
Comment on lines 158 to 167
func getTermSigalContext() (ctx context.Context, stop context.CancelFunc) {
return signal.NotifyContext(
context.Background(),
os.Interrupt,
// More Linux signals here
syscall.SIGHUP,
syscall.SIGTERM,
syscall.SIGQUIT,
)
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd counsel moving this to being inline, as it makes it clearer what's happening. Wrapping a single function call in a function call reduces the clarity of what is going on.

Please note, this point is a matter of taste, so feel free to disagree with me here. I offer this as advice, not as 'You need to fix this to get merged'.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edwarnicke agree with your point, have changed back using signal.NotifyContext as inline function call.


starttime := time.Now()

log.FromContext(ctx).Infof("executing phase 1: get config from environment (time since start: %s)", time.Since(starttime))
Copy link
Member

Choose a reason for hiding this comment

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

Once again, as a matter of personal taste, I find this style: https://github.com/networkservicemesh/cmd-forwarder-vpp/blob/e9103cf238a4b5c8449ef04170c4ccf78cbce523/main.go#L111-L113 easier to read... you are more than welcome to have a differing opinion :)

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, fixed it accordingly without making changes to linter settings. hope its ok now.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@edwarnicke edwarnicke merged commit 303cb93 into networkservicemesh:main Aug 17, 2021
@pperiyasamy pperiyasamy deleted the main-module branch August 17, 2021 14:24
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