feat: coordination session client #830

arkhipov · 2023-09-19T15:12:15Z

Introduce coordination service Session synchronous API.

Pull request type

What is the current behavior?

There is no coordination session client.

Issue Number: 53

What is the new behavior?

There is a synchronous coordination session API client.

Other information

WIP! PLEASE DO NOT MERGE!

coordination/coordination.go

Introduce Coordination Service Session synchronous API. For now, it supports operations that do not create subscriptions (though, the DescribeSemaphore method still can be used to observe owners and waiters of a semaphore). The Session object is a high-level abstraction of the session concept, which is introduced in the Coordination Service API. The client takes over and controls the network, connection and server issues. It also seamlessly integration with the Golang context concept using the Lease object which context is alive until the corresponding semaphore is considered acquired. See also #53

This contains the implementation of the Session coordination client and related objects that may be used to implement a typical client for a conversation-like protocol based on a bidirectional gRPC stream. See also #53

- Locks: shows how to use the Lease object and its context to make code execute at one node in a cluster at a time. - Workers: shows how use the Coordination Service to distribute independent tasks among multiple workers. See also #53

snaury · 2023-10-02T10:59:13Z

coordination/coordination.go

+	// UpdateSemaphore changes semaphore data. This method waits until the server successfully updates the semaphore or
+	// returns an error.
+	//
+	// This method is idempotent. The client will automatically retry in the case of network or server failure.


Perhaps it shouldn't be idempotent? Since Create and Delete are not idempotent, it looks like an odd one.

This one is tricky. Although it's not idempotent in general, in most cases users synchronize access to this method in order to avoid data races so that it becomes idempotent. I can hardly imagine why someone might want to update semaphore data without proper synchronization. I suggest changing this comment to explain that. However, I think it is feasible to let the client automatically retry this method calls and have an option of turning them off. What do you think of that?

snaury · 2023-10-02T11:01:31Z

coordination/coordination.go

+
+	// DescribeSemaphore returns the state of the semaphore.
+	//
+	// This method is idempotent. The client will automatically retry in the case of network or server failure.


I think at least Describe with watches cannot be idempotent, but you don't seem to support them. Are you planning to add watches as a separate method?

At the moment, there are no watches in the client, so it should be idempotent. I've been thinking about supporting this feature, however I'm not sure what the API will look like.

asmyasnikov · 2023-10-02T16:50:17Z

examples/coordination/README.md

+		// The context is canceled.
+        break		
+    }
+


defer session.Close() ?

examples/coordination/lock/main.go

codecov-commenter · 2023-10-03T00:49:33Z

Codecov Report

Attention: 413 lines in your changes are missing coverage. Please review.

Comparison is base (ed57546) 67.50% compared to head (a000ca4) 67.93%.

Files	Patch %	Lines
internal/coordination/session.go	67.31%	177 Missing and 25 partials ⚠️
log/coordination.go	49.68%	65 Missing and 14 partials ⚠️
internal/coordination/conversation/conversation.go	67.22%	69 Missing and 9 partials ⚠️
coordination/options/options.go	26.66%	44 Missing ⚠️
internal/coordination/client.go	76.74%	7 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
+ Coverage   67.50%   67.93%   +0.42%     
==========================================
  Files         252      256       +4     
  Lines       24508    25629    +1121     
==========================================
+ Hits        16545    17410     +865     
- Misses       7100     7318     +218     
- Partials      863      901      +38

Flag	Coverage Δ
	`55.10% <63.35%> (+1.10%)`	⬆️
go-1.20.x	`67.68% <62.55%> (+0.31%)`	⬆️
go-1.21.x	`67.91% <63.35%> (+0.51%)`	⬆️
integration	`55.10% <63.35%> (+1.10%)`	⬆️
macOS	`38.50% <0.00%> (-0.35%)`	⬇️
ubuntu	`38.51% <0.00%> (-0.33%)`	⬇️
unit	`38.62% <0.00%> (-0.31%)`	⬇️
windows	`38.61% <0.00%> (-0.25%)`	⬇️
ydb-22.5	`54.68% <63.35%> (+1.02%)`	⬆️
ydb-23.1	`54.64% <62.55%> (+1.12%)`	⬆️
ydb-23.2	`54.81% <63.35%> (+1.23%)`	⬆️
ydb-23.3	`54.88% <63.35%> (+1.22%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rekby · 2023-10-06T18:53:39Z

coordination/coordination.go

+	// - call Close on the Session,
+	// - close the Client which the session was created with,
+	// - call any method of the Session until the ErrSessionClosed is returned.
+	OpenSession(ctx context.Context, path string, opts ...options.OpenSessionOption) (Session, error)


In the sdk and in other methods of the interface ctx is cancel token for current method call.

Timeout for the request is unusual for go: by option.

What about to use ctx in the OpenSession same way - for set cancel context/timeout of OpenSession method.

And add option for set additional context for lifetime control.

And lets to return struct/struct pointer, not interface.

Any change in interface (including add any method) - is brokable changing.
It will difficult to extend API of the Session if it will the interface.

In the sdk and in other methods of the interface ctx is cancel token for current method call.

Timeout for the request is unusual for go: by option.

What about to use ctx in the OpenSession same way - for set cancel context/timeout of OpenSession method.

And add option for set additional context for lifetime control.

It looks a bit dangerous to me since it may cause session leaks. Though, if the whole SDK uses context that way, I think it's better to be consistent. I haven't found a way to do that for Driver or Table.Session, what are you suggestions on naming the option?

rekby · 2023-10-06T19:09:32Z

coordination/coordination.go

+// Lease is the object which defines the rights of the session to the acquired semaphore. Lease is alive until its
+// context is not canceled. This may happen implicitly, when the associated session becomes lost or closed, or
+// explicitly, if someone calls the Release method of the lease.
+type Lease interface {


Same as Session - let return the type, not interface

rekby · 2023-10-06T19:16:43Z

coordination/options/options.go

+// it is still waiting in the queue.
+//
+// This is the default behavior. You can set the specific timeout by calling the WithAcquireTimeout method.
+func WithNoAcquireTimeout() AcquireSemaphoreOption {


NoTimeout can mean two things:

Wait infinite time

No wait and return immediately

What about rename the method to WithAquireInfiniteTimeout?

I think it not useful anymore since there is the MaxTimeout constant in the coordination package.

rekby · 2023-10-06T19:21:24Z

examples/coordination/README.md

+    <-session.Context().Done()
+
+    // The tasks must be stopped since we do not own the semaphores anymore.
+    cancelTasks()


Why we need explicit stop tasks?

we call doWork with lease.Context() and the context will be cancelled when session will be closed (session context cancelled)

Good call! I'll change this to 'wait for the tasks to complete'

asmyasnikov · 2023-12-10T16:34:03Z

What do you think about suggestion #907 ?

arkhipov marked this pull request as ready for review September 22, 2023 08:15

arkhipov added 2 commits September 24, 2023 14:13

Merge branch 'ydb-platform:master' into master

991eb73

Merge branch 'ydb-platform:master' into master

f9da194

asmyasnikov reviewed Sep 25, 2023

View reviewed changes

coordination/coordination.go Outdated Show resolved Hide resolved

coordination/coordination.go Outdated Show resolved Hide resolved

arkhipov added 3 commits October 2, 2023 12:45

feat: implement coordination session client

5356cba

This contains the implementation of the Session coordination client and related objects that may be used to implement a typical client for a conversation-like protocol based on a bidirectional gRPC stream. See also #53

feat: add coordination service client examples

0eff713

- Locks: shows how to use the Lease object and its context to make code execute at one node in a cluster at a time. - Workers: shows how use the Coordination Service to distribute independent tasks among multiple workers. See also #53

snaury reviewed Oct 2, 2023

View reviewed changes

asmyasnikov reviewed Oct 2, 2023

View reviewed changes

rekby reviewed Oct 6, 2023

View reviewed changes

Merge branch 'master' into master

a000ca4

asmyasnikov force-pushed the master branch from 7be8c10 to c6a7a8b Compare March 11, 2024 12:34

Merge branch 'master' into master

1948cf3

asmyasnikov changed the base branch from master to coordination March 25, 2024 08:35

asmyasnikov merged commit b19bdf4 into ydb-platform:coordination Mar 25, 2024
29 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: coordination session client #830

feat: coordination session client #830

arkhipov commented Sep 19, 2023 •

edited

Loading

snaury Oct 2, 2023

arkhipov Oct 9, 2023

snaury Oct 2, 2023

arkhipov Oct 9, 2023

asmyasnikov Oct 2, 2023

codecov-commenter commented Oct 3, 2023 •

edited

Loading

rekby Oct 6, 2023

rekby Oct 6, 2023

arkhipov Oct 9, 2023 •

edited

Loading

rekby Oct 6, 2023

rekby Oct 6, 2023

arkhipov Oct 9, 2023

rekby Oct 6, 2023

arkhipov Oct 9, 2023

asmyasnikov commented Dec 10, 2023

feat: coordination session client #830

feat: coordination session client #830

Conversation

arkhipov commented Sep 19, 2023 • edited Loading

Pull request type

What is the current behavior?

What is the new behavior?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkhipov Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmyasnikov commented Dec 10, 2023

arkhipov commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Oct 3, 2023 •

edited

Loading

arkhipov Oct 9, 2023 •

edited

Loading