#1008: Added Q.WATCH support to Websocket #1090

psrvere · 2024-10-14T11:47:14Z

Closes #1008

This PR

adds support for Q.WATCH command on Websocket server
changes Websocket server logic
a. updates WebsocketServer struct to use ioChan and qwatchResponseChan
b. extracts out response processing logic to a function
c. adds a function to process Q.WATCH updates
changes command parsing logic in ParseWebsocketMessage to support Q.WATCH command
adds more unit tests to increase coverage of parsing logic
adds unit tests for Q.WATCH
adds integration tests for Q.WATCH
updates Q.WATCH docs

psrvere · 2024-10-16T07:17:16Z

@lucifercr07 @pratikpandey21 - Added Qwatch to Websocket. Please review.

pratikpandey21

I'd recommend waiting for #1086
and leverage the code there as that would enable multithreading in WS.

Will review once those changes are there

internal/server/utils/redisCmdAdapter_test.go

lucifercr07 · 2024-10-18T08:09:05Z

internal/server/websocketServer.go

 	for {
 		// read incoming message
 		_, msg, err := conn.ReadMessage()
 		if err != nil {
-			writeResponse(conn, []byte("error: command reading failed"))
-			continue
+			break
 		}

 		// parse message to dice command
 		diceDBCmd, err := utils.ParseWebsocketMessage(msg)
 		if errors.Is(err, diceerrors.ErrEmptyCommand) {
 			continue


Shall we log a warn here? Or return an error to client. As client is just receiving an empty response in this case which doesn't seem to be correct.

Added a warning here.

@psrvere not able to see the log statement added here, is commit pushed?

@lucifercr07 - the warning was added as part of #1142 - please see here

That was important for integration tests to work so had to move to that PR.

internal/server/websocketServer.go

internal/server/utils/redisCmdAdapter_test.go

lucifercr07 · 2024-10-18T08:37:00Z

internal/server/websocketServer.go

-			"-2",     // Represents a RESP Integer with value -2.
-			"*0",     // Represents an empty RESP Array.
+			// start a goroutine for subsequent updates
+			go s.processQwatchUpdates(clientIdentifierID, conn, diceDBCmd)


Shall we refactor this such that we don't spawn new go routine for each qwatch cmd subscription?
We can create another channel at start for QWATCH and keep registering clients to it and handle all responses in a single event loop.

This can also be taken as enhancement on top of this PR, not a blocker.

Yes, this is absolutely critical. I realised this while writing tests with multiple subscribers. Have created a follow up issue #1153

lucifercr07 · 2024-10-18T08:38:56Z

internal/server/websocketServer.go

 			}
+		case <-s.shutdownChan:
+			return


Shall we explicitly send qunwatch command from here and de-register the client?
Currently the cleanup happens after the key is triggered with a error message as websocket closed.

Sure. Let me also figure out how q.unwatch will work in websocket. Will address this comment as part of #1154

lucifercr07 · 2024-10-18T08:39:35Z

integration_tests/commands/websocket/qwatch_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestQWatch(t *testing.T) {


Let's add a test where multiple subscribers are there for query and responses are triggered for them.

Added the test but with single subscriber right now. As part of #1153, I will increase the number of subscribers to 5 (arbitrary)

This test also will come as part of #1153. Currently previous tests leave a subscriber which also receive response when value of a key is updated. So it become multi subscriber problem again.

psrvere · 2024-10-19T11:35:14Z

@lucifercr07 - have addressed all comments and created new issues where needed. Please review again.

lucifercr07 · 2024-10-20T09:31:09Z

internal/server/websocketServer.go

 	for {
 		// read incoming message
 		_, msg, err := conn.ReadMessage()
 		if err != nil {
-			writeResponse(conn, []byte("error: command reading failed"))
-			continue
+			break
 		}

 		// parse message to dice command
 		diceDBCmd, err := utils.ParseWebsocketMessage(msg)
 		if errors.Is(err, diceerrors.ErrEmptyCommand) {
 			continue


@psrvere not able to see the log statement added here, is commit pushed?

psrvere · 2024-10-20T10:02:27Z

@lucifercr07 - merging with latest master and resolving a few conflicts.

psrvere · 2024-10-20T10:40:14Z

@lucifercr07 - good to go, please check.

lucifercr07

LGTM

psrvere added 9 commits October 14, 2024 17:07

updated parsing logic to support qwatch and qunwatch commands

3ca4c51

update websocket server logic to support qwatch and qunwatch

0feea23

added integration tests

53bf950

add qunwatch unit test

a8c99fe

fixed linter warnings

b833052

remove qunwatch code

be7132a

keep qunwatch as unimplemented command

b0ee3a6

merged with master

3bbf747

fixed comment

edc7203

psrvere changed the title ~~Added QWATCH and QUNWATCH support to Websocket~~ Added QWATCH support to Websocket Oct 14, 2024

psrvere added 10 commits October 15, 2024 12:29

debugging

e39c9a0

minor fixes

4e6dbe9

moved test port to a constant

1e91489

debugging

7307273

minor fixes

096114d

fixed integration tests

7331d91

fixed linter checks

c3a087c

Merge branch 'master' into websocket-qwatch

bb41309

fixed breaking tests

39687ef

fixed linter warnings

faee66b

psrvere marked this pull request as ready for review October 16, 2024 07:09

This was referenced Oct 16, 2024

Add QWatch support to Websocket #1008

Closed

POC for WebSocket Integration with DiceDB #556

Open

psrvere added 6 commits October 17, 2024 12:49

merged with master

95baab4

renamed qwatch to q.watch

7a389a5

fixed qwatch tests

ec04608

fixed json tests

bd64c93

fixed unit tests to use Q.WATCH

4e44c01

minor fix

0544da0

pratikpandey21 reviewed Oct 17, 2024

View reviewed changes

internal/server/utils/redisCmdAdapter_test.go Show resolved Hide resolved

psrvere changed the title ~~Added QWATCH support to Websocket~~ Added Q.WATCH support to Websocket Oct 18, 2024

lucifercr07 requested changes Oct 18, 2024

View reviewed changes

psrvere added 4 commits October 18, 2024 15:16

Merge branch 'master' into websocket-qwatch

f7a8bc4

added warning failing to read message from client

97a4496

updated docs

4fc97a2

merged with master

96f10e0

psrvere mentioned this pull request Oct 19, 2024

Handle Q.WATCH updates in a single event loop for Websockets #1153

Open

minor fix

a90fceb

psrvere mentioned this pull request Oct 19, 2024

Add Q.UNWATCH for websocket #1154

Open

psrvere requested a review from lucifercr07 October 19, 2024 11:35

psrvere and others added 2 commits October 19, 2024 18:14

Merge branch 'master' into websocket-qwatch

930b5d9

Merge branch 'master' into websocket-qwatch

e281f70

lucifercr07 requested changes Oct 20, 2024

View reviewed changes

psrvere requested a review from lucifercr07 October 20, 2024 09:59

Merge branch 'master' into websocket-qwatch

5a879ab

lucifercr07 approved these changes Oct 20, 2024

View reviewed changes

kakdeykaushik mentioned this pull request Oct 20, 2024

Add support for command CLIENT LIST #1134

Open

JyotinderSingh changed the title ~~Added Q.WATCH support to Websocket~~ #1008: Added Q.WATCH support to Websocket Oct 21, 2024

JyotinderSingh merged commit 846cef3 into DiceDB:master Oct 21, 2024
2 checks passed

This was referenced Oct 21, 2024

Handle Q.WATCH Updates in a single event loop for Websockets #1173

Closed

Handle Q.WATCH updates in a single event loop for Websockets #1175

Open

KanniShashankh pushed a commit to KanniShashankh/dice that referenced this pull request Oct 21, 2024

DiceDB#1008: Added Q.WATCH support to Websocket (DiceDB#1090)

147516a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1008: Added Q.WATCH support to Websocket #1090

#1008: Added Q.WATCH support to Websocket #1090

psrvere commented Oct 14, 2024 •

edited

Loading

psrvere commented Oct 16, 2024

pratikpandey21 left a comment

lucifercr07 Oct 18, 2024

psrvere Oct 18, 2024

lucifercr07 Oct 20, 2024

psrvere Oct 20, 2024

lucifercr07 Oct 18, 2024

psrvere Oct 19, 2024

lucifercr07 Oct 18, 2024

psrvere Oct 19, 2024

lucifercr07 Oct 18, 2024

psrvere Oct 19, 2024 •

edited

Loading

psrvere Oct 19, 2024

psrvere commented Oct 19, 2024

lucifercr07 Oct 20, 2024

psrvere commented Oct 20, 2024 •

edited

Loading

psrvere commented Oct 20, 2024

lucifercr07 left a comment

#1008: Added Q.WATCH support to Websocket #1090

#1008: Added Q.WATCH support to Websocket #1090

Conversation

psrvere commented Oct 14, 2024 • edited Loading

psrvere commented Oct 16, 2024

pratikpandey21 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psrvere Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psrvere commented Oct 19, 2024

Choose a reason for hiding this comment

psrvere commented Oct 20, 2024 • edited Loading

psrvere commented Oct 20, 2024

lucifercr07 left a comment

Choose a reason for hiding this comment

psrvere commented Oct 14, 2024 •

edited

Loading

psrvere Oct 19, 2024 •

edited

Loading

psrvere commented Oct 20, 2024 •

edited

Loading