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

[sdk#1026] Use postpone.ContextWithValues() #300

Merged

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Jul 26, 2021

Description

Use postpone.ContextWithValues() for Close/Unregister in failed Request/Register cases.

Depends on networkservicemesh/sdk#1035.

Issue link

networkservicemesh/sdk#1026

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch from d3fcbb0 to cf6c8c3 Compare August 19, 2021 05:41
@Bolodya1997 Bolodya1997 marked this pull request as ready for review August 19, 2021 05:43
@Bolodya1997 Bolodya1997 changed the title [sdk#1026] Close context [sdk#1026] Use postpone.ContextWithValues() Aug 19, 2021
func (c *injectClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest,
opts ...grpc.CallOption) (*networkservice.Connection, error) {
logger := log.FromContext(ctx).WithField("injectClient", "Request")
func (c *injectClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
Copy link
Member

Choose a reason for hiding this comment

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

can you also make this change for inject server ?

Copy link
Author

Choose a reason for hiding this comment

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

Inject server first make move and after makes Request, so it doesn't need postpone.
Or can you please point what changes do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about this one, but I guess using closeCtx here for ifMoveBack doesn't have any impact here. please just ignore this comment.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the comment, you are totally right - we should use postpone not only for the Close but also for closing own resources.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@Bolodya1997 Bolodya1997 marked this pull request as draft August 24, 2021 13:02
@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch from cf6c8c3 to b66e5d7 Compare August 25, 2021 04:15
@Bolodya1997 Bolodya1997 marked this pull request as ready for review August 25, 2021 04:15
@Bolodya1997 Bolodya1997 marked this pull request as draft August 25, 2021 04:17
@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch from b66e5d7 to b721118 Compare August 25, 2021 04:18
@Bolodya1997 Bolodya1997 marked this pull request as ready for review August 25, 2021 04:18
@denis-tingaikin denis-tingaikin merged commit 26a9371 into networkservicemesh:main Aug 25, 2021
nsmbot pushed a commit to networkservicemesh/sdk-vpp that referenced this pull request Aug 25, 2021
…k-kernel@main

PR link: networkservicemesh/sdk-kernel#300

Commit: 26a9371
Author: Denis Tingaikin
Date: 2021-08-26 02:24:38 +0300
Message:
  - Merge pull request #300 from Bolodya1997/sdk#1026/close-context
Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/sdk-sriov that referenced this pull request Aug 25, 2021
…k-kernel@main

PR link: networkservicemesh/sdk-kernel#300

Commit: 26a9371
Author: Denis Tingaikin
Date: 2021-08-26 02:24:38 +0300
Message:
  - Merge pull request #300 from Bolodya1997/sdk#1026/close-context
Signed-off-by: NSMBot <[email protected]>
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