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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Aug 26, 2021
Merged

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

merged 2 commits into from
Aug 26, 2021

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Jul 22, 2021 •
edited
Loading

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
Copy link
Author

@edwarnicke
Looks like we need to pass ctx to almost every chain element. What do you think about creating some basectx chain elements injecting application context to the Request context?

@edwarnicke
Copy link
Member

Rather that injecting it... just write a very simple utility function to create it given a context.

Think about the understandability of the resulting code.

I can either call some function that is hopefully as close as possible to existing Context idioms, or I can fish something out of the existing context. If I want to go back and figure out what is going on... which is easier to understand?

@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch 3 times, most recently from 694b5bf to 09870be Compare July 23, 2021 15:33
@Bolodya1997
Copy link
Author

@edwarnicke
You are totally right, we don't actually need application context, because context.Background() solves the same issue and doesn't need to be injected to the chain element.

@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch from 09870be to 933437b Compare August 19, 2021 05:21
@Bolodya1997 Bolodya1997 changed the title [sdk#1026] Use closectx [sdk#1026] Use postpone.ContextWithValues() Aug 19, 2021
@Bolodya1997 Bolodya1997 marked this pull request as ready for review August 19, 2021 05:23
@denis-tingaikin
Copy link
Member

LGTM

@edwarnicke Do you still have any unresolved comments on this PR?

@Bolodya1997 Bolodya1997 marked this pull request as draft August 24, 2021 13:02
Vladimir Popov added 2 commits August 25, 2021 11:40
Use postpone.ContextWithValues() for Close on failure cases
8f43bf8 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Use postpone for closing internal resources
4c8daa2 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 force-pushed the sdk#1026/close-context branch from 933437b to 4c8daa2 Compare August 25, 2021 04:40
@Bolodya1997 Bolodya1997 marked this pull request as ready for review August 25, 2021 04:41
@denis-tingaikin
Copy link
Member

Merging this.

@edwarnicke Feel free to ping us if we missed unresolved comments. Changes based on networkservicemesh/sdk#1035

@denis-tingaikin denis-tingaikin merged commit cfdbe19 into networkservicemesh:main Aug 26, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder-vpp that referenced this pull request Aug 26, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
5eb3e7b 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#322

Commit: cfdbe19
Author: Vladimir Popov
Date: 2021-08-27 06:18:32 +0700
Message:
  - [sdk#1026] Use postpone.ContextWithValues() (#322)
* Use postpone.ContextWithValues() for Close on failure cases

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Use postpone for closing internal resources

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-vpp that referenced this pull request Aug 26, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
72ae4d4 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#322

Commit: cfdbe19
Author: Vladimir Popov
Date: 2021-08-27 06:18:32 +0700
Message:
  - [sdk#1026] Use postpone.ContextWithValues() (#322)
* Use postpone.ContextWithValues() for Close on failure cases

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Use postpone for closing internal resources

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-forwarder-vpp that referenced this pull request Aug 26, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
1c70c08 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#322

Commit: cfdbe19
Author: Vladimir Popov
Date: 2021-08-27 06:18:32 +0700
Message:
  - [sdk#1026] Use postpone.ContextWithValues() (#322)
* Use postpone.ContextWithValues() for Close on failure cases

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Use postpone for closing internal resources

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Aug 26, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin approved these changes

@glazychev-art glazychev-art glazychev-art approved these changes

@Mixaster995 Mixaster995 Awaiting requested review from Mixaster995

Assignees

@Bolodya1997 Bolodya1997

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bolodya1997 @edwarnicke @denis-tingaikin @glazychev-art