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

Rework client to use chain elements #15

Merged
merged 10 commits into from
Nov 20, 2020

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Sep 16, 2020

Issues:

  1. We already have kernel.NewClient(), vfio.NewClient() to add client chain elements, no need to duplicate such code.
  2. Existing design doesn't allow client to make request with multiple MechanismPreferences set.

Solution:

  1. Reuse existing client chain elements and append them to client chain.
  2. Merge multiple NetworkServiceConfig by network service name to single request.

@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 2 times, most recently from 5a46690 to cad3186 Compare September 16, 2020 07:00
@Bolodya1997 Bolodya1997 marked this pull request as draft September 18, 2020 06:36
@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 3 times, most recently from 881e985 to 56bfef7 Compare September 18, 2020 07:54
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 6 times, most recently from effd8e7 to d4adca8 Compare September 25, 2020 05:52
@Bolodya1997 Bolodya1997 marked this pull request as ready for review September 25, 2020 08:21
@Bolodya1997 Bolodya1997 marked this pull request as draft September 25, 2020 08:27
@Bolodya1997 Bolodya1997 marked this pull request as ready for review September 25, 2020 16:04
@Bolodya1997 Bolodya1997 marked this pull request as draft October 7, 2020 06:55
@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 3 times, most recently from 8fd7cfa to f7dbef9 Compare October 20, 2020 10:20
@Bolodya1997 Bolodya1997 marked this pull request as ready for review October 20, 2020 10:24
@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 4 times, most recently from 4d1e6bd to fb01e43 Compare October 30, 2020 06:04
README.md Outdated Show resolved Hide resolved
@Bolodya1997 Bolodya1997 force-pushed the client-chains branch 5 times, most recently from 94274ef to 6514139 Compare November 13, 2020 09:30
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Vladimir Popov added 6 commits November 17, 2020 10:43
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 marked this pull request as draft November 17, 2020 08:24
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 marked this pull request as ready for review November 17, 2020 08:51
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

Added few minor comments.
Will merge this when this repo will be updated by https://github.com/networkservicemesh/cmd-template (Waiting for PR networkservicemesh/cmd-template#45)

.golangci.yml Outdated
@@ -169,3 +169,13 @@ issues:
exclude-use-default: false
max-issues-per-linter: 0
max-same-issues: 0
exclude-rules:
Copy link
Member

Choose a reason for hiding this comment

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

Probably we need to disable one of two linters if they are conflicted

Copy link
Author

Choose a reason for hiding this comment

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

Yep, don't actually sure that sloppyReassign is any useful, have disabled it.

main.go Outdated
)

const (
cleanupTimeout = 10 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Can we use config.ConnectTimeout ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can.

Vladimir Popov added 2 commits November 20, 2020 09:23
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@denis-tingaikin denis-tingaikin merged commit 4457c18 into networkservicemesh:master Nov 20, 2020
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