Rework adapters #543

xzfc · 2020-10-19T12:03:37Z

This PR fixes several problems in adapters:

In nested adapters case, the old adapter may run some requests (a) in the wrong order, or (b) repeat them twice or more. See Rework adapters to prevent calling chain elements twice #535 (comment)
Nesting order. Adapters are now nest requests in the same way as chain and next do. See Rework adapters to prevent calling chain elements twice #535 (comment)
Adapters are now thread-safe.

Tests contain two checks that look similar but serve different purposes:

Context passing: check that the adapter passes the context correctly.
Execution order: check that each chain element is called exactly once (or never called if some chain element intentionally doesn't call the next chain element). Also, it checks the nesting order.

Closes #535.

xzfc · 2020-10-19T12:10:07Z

pkg/networkservice/common/updatetoken/server_test.go

@@ -224,7 +224,7 @@ func (f *updateTokenServerSuite) TestChain() {

 	got, err := server.Request(context.Background(), request)
 	require.Equal(t, 3, len(got.Path.PathSegments))
-	require.Equal(t, 2, int(got.Path.Index))
+	require.Equal(t, 0, int(got.Path.Index))


Context:

sdk/pkg/networkservice/common/updatetoken/server_test.go

Lines 214 to 227 in 217f0e0

elements := []networkservice.NetworkServiceServer{

adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("nsc-1"), updatetoken.NewClient(TokenGenerator))),

updatepath.NewServer("local-nsm-1"),

updatetoken.NewServer(TokenGenerator),

adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("local-nsm-1"), updatetoken.NewClient(TokenGenerator))),

updatepath.NewServer("remote-nsm-1"),

updatetoken.NewServer(TokenGenerator),

adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("remote-nsm-1"), updatetoken.NewClient(TokenGenerator)))}

server := next.NewNetworkServiceServer(elements...)

got, err := server.Request(context.Background(), request)

require.Equal(t, 3, len(got.Path.PathSegments))

require.Equal(t, 0, int(got.Path.Index))

Path.Index is now restored by updatetoken on line 215.
This is changed because of the restored nesting order.

pkg/networkservice/core/adapters/networkservice.go

denis-tingaikin · 2020-10-19T12:20:35Z

pkg/networkservice/core/adapters/networkservice_test.go

+	return next.Client(ctx).Close(ctx, in, opts...)
+}
+
+func appendContext(ctxPtr *context.Context, prefix, str string) func() {


I think strings.Builder could reduce strings copying for these tests.

These tests run in 0.038s on CI, so I think it doesn't worth optimizing.

As I can see these test run in 1s on windows

github.com/networkservicemesh/sdk/pkg/networkservice/core/adapters 1.044s

It is not super critical but It could be faster.

On windows, every test run in at least one second, even such simple as clienturl. It looks like every test on windows delayed exactly by 1 second (or time measured incorrectly).

denis-tingaikin

Also, I think we should apply the same patch for registry chains https://github.com/networkservicemesh/sdk/tree/master/pkg/registry/core/adapters.

pkg/networkservice/core/adapters/client_to_server.go

xzfc · 2020-10-20T07:41:25Z

Ready to merge.

Applying the same patch for registry chains (#543 (review)) isn't straightforward because we have to deal with gRPC streams in the Find method, so it's better to be done in a separate PR.

xzfc · 2020-10-20T12:04:32Z

Update: slightly reworked, now adapters use next.NewNetworkServiceClient instead of context.WithValue to pass the client. (and same for server) The basic logic remains the same.

Reason: make adapters resistant to context clearing. I.e. the following code works fine now:

next.Client(ctx).Request(context.Background() /* ← note this */, request, args...)

edwarnicke · 2020-10-21T01:54:41Z

Update: slightly reworked, now adapters use next.NewNetworkServiceClient instead of context.WithValue to pass the client. (and same for server) The basic logic remains the same.

Reason: make adapters resistant to context clearing. I.e. the following code works fine now:
next.Client(ctx).Request(context.Background() /* ← note this */, request, args...)

Oh... that's slick :)

denis-tingaikin

In general, I like these changes.
I think also we can add a few simple benchmark tests to track the performance of adapters.

pkg/registry/core/adapters/nse_next_server.go

pkg/registry/core/adapters/ns_registry.go

xzfc · 2020-10-26T04:48:01Z

Benchmarks on my machine, comparing the old and new implementations.

`pkg/networkservice/core/adapters`	Old	New1	New2
`BenchmarkServerToClient-8`	686	596	598
`BenchmarkClientToServer-8`	700	589	587
`BenchmarkDeepNested-8`	96000	12669	12704

`pkg/registry/core/adapters`	Old	New2
`BenchmarkNSServerFind-8`	1679	1705
`BenchmarkNSClientFind-8`	2725	2669
`BenchmarkNSEServerFind-8`	1684	1747
`BenchmarkNSEClientFind-8`	2677	2694

Times in ns/op.
Old is f7fb405 (from master).
New1 is the first context-based adapters (previous version of this PR).
New2 is the next-based adapters (the current version of this PR).
Only the Find method benchmarked in registry adapters, as the rest should be the same as in the core benchmarks.

denis-tingaikin · 2020-10-26T08:46:03Z

@xzfc could you add the current benchmark result into README.md of pkg?

denis-tingaikin

LGTM

denis-tingaikin · 2020-10-28T05:41:35Z

pkg/networkservice/core/adapters/README.md

@@ -0,0 +1,9 @@
+This package provides adapters to translate between `NetworkServiceServer` and `NetworkServiceClient` interfaces.


@xzfc Could you add the same README.md file for registry adapters?

denis-tingaikin · 2020-10-28T05:41:45Z

pkg/networkservice/core/adapters/README.md

@@ -0,0 +1,9 @@
+This package provides adapters to translate between `NetworkServiceServer` and `NetworkServiceClient` interfaces.


Suggested change

This package provides adapters to translate between `NetworkServiceServer` and `NetworkServiceClient` interfaces.

## Intro

This package provides adapters to translate between `NetworkServiceServer` and `NetworkServiceClient` interfaces.

Signed-off-by: Albert Safin <albert.safin@xored.com>

…k@master networkservicemesh/sdk#543 networkservicemesh/sdk PR link: networkservicemesh/sdk#543 networkservicemesh/sdk commit message: commit dfb11c96a95752aee842105d70c7f4dbe0dc7c60 Author: xzfc <xzfc@users.noreply.github.com> Date: Wed Oct 28 06:31:10 2020 +0000 Rework adapters (#543) * NS adapters: rework, update tests, add benchmarks and readme Signed-off-by: Albert Safin <albert.safin@xored.com> * NS adapters: use next instead of context Signed-off-by: Albert Safin <albert.safin@xored.com> * Registry adapters: rework Signed-off-by: Albert Safin <albert.safin@xored.com> * Registry adapters: add benchmarks and readme Signed-off-by: Albert Safin <albert.safin@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

* NS adapters: rework, update tests, add benchmarks and readme Signed-off-by: Albert Safin <albert.safin@xored.com> * NS adapters: use next instead of context Signed-off-by: Albert Safin <albert.safin@xored.com> * Registry adapters: rework Signed-off-by: Albert Safin <albert.safin@xored.com> * Registry adapters: add benchmarks and readme Signed-off-by: Albert Safin <albert.safin@xored.com> Signed-off-by: Sergey Ershov <sergey.ershov@xored.com>

xzfc commented Oct 19, 2020

View reviewed changes

denis-tingaikin reviewed Oct 19, 2020

View reviewed changes

edwarnicke reviewed Oct 19, 2020

View reviewed changes

pkg/networkservice/core/adapters/client_to_server.go Outdated Show resolved Hide resolved

denis-tingaikin reviewed Oct 22, 2020

View reviewed changes

pkg/registry/core/adapters/nse_next_server.go Outdated Show resolved Hide resolved

pkg/registry/core/adapters/ns_registry.go Outdated Show resolved Hide resolved

pkg/registry/core/adapters/ns_registry.go Outdated Show resolved Hide resolved

denis-tingaikin approved these changes Oct 26, 2020

View reviewed changes

denis-tingaikin approved these changes Oct 27, 2020

View reviewed changes

denis-tingaikin requested review from edwarnicke and haiodo October 27, 2020 14:09

denis-tingaikin reviewed Oct 28, 2020

View reviewed changes

xzfc added 4 commits October 28, 2020 06:25

NS adapters: rework, update tests, add benchmarks and readme

afcfc46

Signed-off-by: Albert Safin <albert.safin@xored.com>

NS adapters: use next instead of context

3417f24

Signed-off-by: Albert Safin <albert.safin@xored.com>

Registry adapters: rework

350d360

Signed-off-by: Albert Safin <albert.safin@xored.com>

Registry adapters: add benchmarks and readme

91361d3

Signed-off-by: Albert Safin <albert.safin@xored.com>

denis-tingaikin approved these changes Oct 28, 2020

View reviewed changes

denis-tingaikin merged commit dfb11c9 into networkservicemesh:master Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework adapters #543

Rework adapters #543

xzfc commented Oct 19, 2020

xzfc Oct 19, 2020

denis-tingaikin Oct 19, 2020

xzfc Oct 19, 2020

denis-tingaikin Oct 19, 2020

xzfc Oct 19, 2020

denis-tingaikin left a comment

xzfc commented Oct 20, 2020

xzfc commented Oct 20, 2020

edwarnicke commented Oct 21, 2020

denis-tingaikin left a comment

xzfc commented Oct 26, 2020

denis-tingaikin commented Oct 26, 2020

denis-tingaikin left a comment

denis-tingaikin Oct 28, 2020

denis-tingaikin Oct 28, 2020

	elements := []networkservice.NetworkServiceServer{
	adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("nsc-1"), updatetoken.NewClient(TokenGenerator))),
	updatepath.NewServer("local-nsm-1"),
	updatetoken.NewServer(TokenGenerator),
	adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("local-nsm-1"), updatetoken.NewClient(TokenGenerator))),
	updatepath.NewServer("remote-nsm-1"),
	updatetoken.NewServer(TokenGenerator),
	adapters.NewClientToServer(next.NewNetworkServiceClient(updatepath.NewClient("remote-nsm-1"), updatetoken.NewClient(TokenGenerator)))}

	server := next.NewNetworkServiceServer(elements...)

	got, err := server.Request(context.Background(), request)
	require.Equal(t, 3, len(got.Path.PathSegments))
	require.Equal(t, 0, int(got.Path.Index))

		@@ -0,0 +1,9 @@
		This package provides adapters to translate between `NetworkServiceServer` and `NetworkServiceClient` interfaces.

Rework adapters #543

Rework adapters #543

Conversation

xzfc commented Oct 19, 2020

xzfc Oct 19, 2020

Choose a reason for hiding this comment

denis-tingaikin Oct 19, 2020

Choose a reason for hiding this comment

xzfc Oct 19, 2020

Choose a reason for hiding this comment

denis-tingaikin Oct 19, 2020

Choose a reason for hiding this comment

xzfc Oct 19, 2020

Choose a reason for hiding this comment

denis-tingaikin left a comment

Choose a reason for hiding this comment

xzfc commented Oct 20, 2020

xzfc commented Oct 20, 2020

edwarnicke commented Oct 21, 2020

denis-tingaikin left a comment

Choose a reason for hiding this comment

xzfc commented Oct 26, 2020

denis-tingaikin commented Oct 26, 2020

denis-tingaikin left a comment

Choose a reason for hiding this comment

denis-tingaikin Oct 28, 2020

Choose a reason for hiding this comment

denis-tingaikin Oct 28, 2020

Choose a reason for hiding this comment