internal/grpcsync: Provide an internal-only pub-sub type API #6167

my4-dev · 2023-04-01T06:43:46Z

This PR adds an internal-only generic pub-sub type API.

Please refer to this conversation in #6036 .

RELEASE NOTES: none

internal/genericpubsub/genericpubsub.go

easwars · 2023-04-03T23:01:01Z

@my4-dev : Thanks for doing this.

and rename it to pubsub

my4-dev · 2023-04-06T15:00:17Z

I've finished incorporating our ideas.
Please take a look at this. Thank you.

internal/grpcsync/pubsub.go

Co-authored-by: Easwar Swaminathan <easwars@google.com>

my4-dev · 2023-04-10T14:22:00Z

@easwars : Thank you for your suggestion. I've corrected.

easwars

I think the code is quite close to what I had in mind initially. I'll start reviewing the tests soon.

internal/grpcsync/pubsub.go

internal/grpcsync/pubsub_test.go

internal/grpcsync/pubsub.go

internal/grpcsync/pubsub_test.go

dfawley · 2023-05-31T20:10:31Z

@arvindbr8 is out of town until when @easwars gets back so reassigning to @easwars.

easwars

LGTM. Just some minor nits this time around.

easwars · 2023-06-27T01:42:40Z

internal/grpcsync/pubsub_test.go

+		for i := 0; i < numPublished; i++ {
+			select {
+			case <-ts1.onMsgCh:
+			case <-time.After(defaultTestShortTimeout):


This one should probably use the defaultTestTimeout instead.

easwars · 2023-06-27T01:44:17Z

internal/grpcsync/pubsub_test.go

+	// Register another subscriber and ensure that it receives the last published message.
+	ts2 := newTestSubscriber(numPublished)
+	pubsub.Subscribe(ts2)
+	wantMsgs2 := []int{numPublished - 1}


This could be wantMsgs2 := wantMsgs1[len(wantMsgs1)-1:]

easwars · 2023-06-27T01:46:12Z

internal/grpcsync/pubsub_test.go

+		for i := 0; i < numPublished; i++ {
+			select {
+			case <-ts1.onMsgCh:
+			case <-time.After(defaultTestShortTimeout):


This should be defaultTestTimeout and the same applies to the next goroutine. We generally use defaultTestShortTimeout only in places where we are waiting for events which we expect to not happen in the test. For example, if we are not expecting a message to arrive from the server, we block for this short timeout and if we receive a message from the server in that timeframe, then we declare an error.

my4-dev · 2023-06-27T13:18:55Z

@easwars : Thank you for approving this PR! I've fixed pubsub_test according to your advice.

easwars · 2023-06-27T16:02:02Z

@my4-dev : Thanks for taking care of all the comments.

@arvindbr8 : Could you please be the second reviewer on this one. Thanks.

arvindbr8

A few comments. Could you also please add comments to the test cases. It would really help with readability for other users.

arvindbr8 · 2023-06-27T20:46:34Z

internal/grpcsync/pubsub_test.go

+	}()
+
+	wg.Wait()
+	if isTimeout {


Why do we fail later in the case of a timeout? I believe its better to t.Fatal whenever the reader goroutine times out and fail fast.

Consider the worst case where onMessage() callback is never invoked. We would have to wait for 10 * 10 ms for the test to fail, where as it could have failed in the first run.

Let me know if there was a different consideration for why we need the isTimeout boolean to check this.

Alternatively, you could use a t.Errorf() when a reader loop times out and then do this after the wg.Wait()

if t.Failed() { t.FailNow() }

Whatever we choose, there are trade-offs.

t.Fatal
If we invoke t.Fatal() in the goroutine which is called by the main goroutine, the main isn't cancelled.
So, we can't use this in sub goroutine.

t.Errof & t.Failed()
It is more like Golang than using boolean flag. But many error messages would be displayed if there is a timeout in loops and it would make logs difficult to read.

Could you please share your opinion about that🙇‍♂️

arvindbr8 · 2023-06-27T20:46:36Z

internal/grpcsync/pubsub_test.go

+	go func() {
+		for i := 0; i < numPublished; i++ {
+			select {
+			case <-ts1.onMsgCh:


IIUC, we are trying to check the subscriber receieves the messages in the order they are published. And we know the message that is published in each iteration (which is i := range numPublished). Why dont we just check for the message received here and fail if its not the one that is expected?

that way we can nix wantMsg1 and gotMsg1 variables and also fail faster.

I usually like the idea of always failing fast as possible in test assertions. Here lets say that the first message itself came out of order, we would have to wait until all the messages are received to fail the test.

In this case, the one sub goroutine is used to call Publish() and the other is used to check to see if ts1.OnMessage() function was called the correct number of times.
Both are processed asynchronously.

Also, if we tried to fail faster, it would be more complicated than before.
(I haven't come up with the idea to do yet.)

arvindbr8 · 2023-06-27T20:48:11Z

internal/grpcsync/pubsub_test.go

+		t.Fatalf("Received messages is %v, want %v", gotMsgs1, wantMsgs1)
+	}
+
+	// Register another subscriber and ensure that it receives the last published message.


receives the last published message

-- which we know in this case if numPublished - 1 right? If you go by my comment above, we could also think about nixing wantMsgs2 right?

arvindbr8 · 2023-06-27T20:52:05Z

internal/grpcsync/pubsub_test.go

+			select {
+			case <-ts1.onMsgCh:
+			case <-time.After(defaultTestTimeout):
+				errCh <- fmt.Errorf("")


Since errCh is initialized with size 1, Wouldn't this be a blocking write if there are multiple timeouts since the channel is only read after wg.Wait()?

I would recommend not using an errCh but instead use t.Error() or t.Fatal() in this case? using t.Error might help you check if both subscribers failed in this case.

Wouldn't this be a blocking write if there are multiple timeouts since the channel is only read after wg.Wait()?

This is correct! So we have to fix this problem.

But, as I said the above comment, I'm hesitating using t.Errorf() or t.Fatalf() .

arvindbr8 · 2023-06-27T20:57:09Z

internal/grpcsync/pubsub_test.go

+	}
+}
+
+func (s) TestPubSub_PublishMsgs_BeforeRegisterSub(t *testing.T) {


Is this not already tests in the previous test case here? Maybe remove that check from the previous test case if you would like to have separate test cases for both

They have different preconditions.
Previous one is tested by calling pubsub.Subscribe() at the status that some test subsribers have already added to pubsub.
The other one is tested at the status that no subscriber has added to pubsub.

Considering above, should we remove either?

arvindbr8

Since the test cases are WAI. I'm approving it and taking my comments as an action item.

add genericpubsub package and implementation

dd64d1d

my4-dev mentioned this pull request Apr 1, 2023

Provide an internal-only API to report connectivity state changes on the ClientConn #6036

Closed

add test file and fix bugs in genericpubsub

fdbf826

easwars self-assigned this Apr 3, 2023

easwars added the Type: Feature New features or improvements in behavior label Apr 3, 2023

easwars added this to the 1.55 Release milestone Apr 3, 2023

easwars requested changes Apr 3, 2023

View reviewed changes

internal/genericpubsub/genericpubsub.go Outdated Show resolved Hide resolved

internal/genericpubsub/genericpubsub.go Outdated Show resolved Hide resolved

easwars assigned my4-dev and unassigned easwars Apr 3, 2023

easwars added the Status: Requires Reporter Clarification label Apr 3, 2023

my4-dev added 2 commits April 4, 2023 22:20

move pubsub from genericpubsub to grpcsync

4f83a06

and rename it to pubsub

Change from Tracker to PubSub

9e14b9f

easwars removed the Status: Requires Reporter Clarification label Apr 6, 2023

easwars self-assigned this Apr 6, 2023

easwars requested changes Apr 6, 2023

View reviewed changes

easwars added the Status: Requires Reporter Clarification label Apr 6, 2023

easwars removed their assignment Apr 6, 2023

my4-dev and others added 3 commits April 8, 2023 18:09

Update PubSub struct comment

d5733fd

Co-authored-by: Easwar Swaminathan <easwars@google.com>

rename OnChange to OnMessage

b6f2843

Co-authored-by: Easwar Swaminathan <easwars@google.com>

apply comments

c48bce5

easwars removed the Status: Requires Reporter Clarification label Apr 10, 2023

easwars self-assigned this Apr 10, 2023

easwars requested changes Apr 10, 2023

View reviewed changes

internal/grpcsync/pubsub.go Outdated Show resolved Hide resolved

internal/grpcsync/pubsub.go Outdated Show resolved Hide resolved

internal/grpcsync/pubsub.go Outdated Show resolved Hide resolved

easwars added the Status: Requires Reporter Clarification label Apr 10, 2023

easwars requested changes Apr 10, 2023

View reviewed changes

internal/grpcsync/pubsub_test.go Outdated Show resolved Hide resolved

internal/grpcsync/pubsub.go Outdated Show resolved Hide resolved

internal/grpcsync/pubsub_test.go Outdated Show resolved Hide resolved

internal/grpcsync/pubsub_test.go Outdated Show resolved Hide resolved

my4-dev added 2 commits April 11, 2023 21:55

resolve race conditions and fix comments

42ecca3

Add some documentation

85edc36

github-actions bot added the stale label May 17, 2023

easwars removed Status: Requires Reporter Clarification stale labels May 17, 2023

arvindbr8 assigned arvindbr8 and unassigned easwars May 23, 2023

arvindbr8 self-requested a review May 23, 2023 17:13

dfawley assigned easwars and unassigned my4-dev and arvindbr8 May 31, 2023

dfawley modified the milestones: 1.56 Release, 1.57 Release Jun 2, 2023

easwars approved these changes Jun 27, 2023

View reviewed changes

easwars assigned my4-dev Jun 27, 2023

my4-dev added 2 commits June 27, 2023 22:15

fix the test according to some advice

8b6bd43

Merge branch 'master' into generic-pub-sub-api

cb3d134

easwars assigned arvindbr8 and unassigned my4-dev Jun 27, 2023

arvindbr8 requested changes Jun 27, 2023

View reviewed changes

arvindbr8 assigned my4-dev and unassigned easwars and arvindbr8 Jun 27, 2023

arvindbr8 added the Status: Requires Reporter Clarification label Jun 27, 2023

arvindbr8 approved these changes Jun 30, 2023

View reviewed changes

arvindbr8 removed the Status: Requires Reporter Clarification label Jun 30, 2023

arvindbr8 merged commit 51042db into grpc:master Jun 30, 2023
1 check passed

my4-dev deleted the generic-pub-sub-api branch July 1, 2023 06:26

github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/grpcsync: Provide an internal-only pub-sub type API #6167

internal/grpcsync: Provide an internal-only pub-sub type API #6167

my4-dev commented Apr 1, 2023 •

edited by easwars

Loading

easwars commented Apr 3, 2023

my4-dev commented Apr 6, 2023 •

edited

Loading

my4-dev commented Apr 10, 2023

easwars left a comment

dfawley commented May 31, 2023

easwars left a comment

easwars Jun 27, 2023

easwars Jun 27, 2023

easwars Jun 27, 2023

my4-dev commented Jun 27, 2023

easwars commented Jun 27, 2023

arvindbr8 left a comment

arvindbr8 Jun 27, 2023

my4-dev Jun 30, 2023

arvindbr8 Jun 27, 2023

my4-dev Jun 30, 2023

arvindbr8 Jun 27, 2023

arvindbr8 Jun 27, 2023

my4-dev Jun 30, 2023 •

edited

Loading

arvindbr8 Jun 27, 2023

my4-dev Jun 30, 2023

arvindbr8 left a comment

internal/grpcsync: Provide an internal-only pub-sub type API #6167

internal/grpcsync: Provide an internal-only pub-sub type API #6167

Conversation

my4-dev commented Apr 1, 2023 • edited by easwars Loading

easwars commented Apr 3, 2023

my4-dev commented Apr 6, 2023 • edited Loading

my4-dev commented Apr 10, 2023

easwars left a comment

Choose a reason for hiding this comment

dfawley commented May 31, 2023

easwars left a 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

my4-dev commented Jun 27, 2023

easwars commented Jun 27, 2023

arvindbr8 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

my4-dev Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

my4-dev commented Apr 1, 2023 •

edited by easwars

Loading

my4-dev commented Apr 6, 2023 •

edited

Loading

my4-dev Jun 30, 2023 •

edited

Loading