Go: Implementing PING command #2264

MikeMwita · 2024-09-10T20:25:23Z

Implemented PING command

Yury-Fridlyand

Cluster client should have ping with 2 optional arguments - message and route

valkey-glide/python/python/glide/async_commands/cluster_commands.py

Lines 174 to 178 in 71d8558

    
               async def ping( 
        
                   self, message: Optional[TEncodable] = None, route: Optional[Route] = None 
        
               ) -> bytes: 
        
                   """ 
        
                   Ping the server.

go/api/base_client.go

Yury-Fridlyand · 2024-09-10T21:19:30Z

go/api/base_client.go


 	resultChannel := make(chan payload)
 	resultChannelPtr := uintptr(unsafe.Pointer(&resultChannel))

+	// If there are no arguments, pass nil for cArgs and argLengths


Where do you check for null? Just to confirm that we're not dereferencing it.

I am checking for null in the executeCommand function, If there are no arguments ( len(cArgs) is 0), I don't dereference the pointers (cArgsPtr and argLengthsPtr ). So they remain nil.

janhavigupta007 · 2024-09-16T05:25:25Z

go/api/commands.go

+
+	// Ping sends the PING command to the server.
+	//
+	// If no argument is provided, returns "PONG". If a message is provided, returns the message.


Let's have two separate implementations of Ping in this case. Ping() with no arguments and PingMessage(message key). I think that would be cleaner.

ping() ping(str)

and for cluster client

ping() ping(str) ping(route) ping(str, route)

janhavigupta007 · 2024-09-16T05:25:31Z

go/api/commands.go

+	//  result, err := client.Ping("Hello")
+	//
+	//[valkey.io]: https://valkey.io/commands/ping/
+	Ping(message string) (string, error)


Ping command is not part of the StringCommands group. Please add a separate interface for ConnectionManagement commands.

janhavigupta007 · 2024-09-16T05:26:38Z

go/api/base_client.go

+	if err != nil {
+		return "", err
+	}
+	return handleStringOrNullResponse(result), nil


Ping command won't return Null response https://valkey.io/commands/ping/.

ravisi21 · 2024-09-16T07:25:42Z

go/api/base_client.go

@@ -213,3 +227,18 @@ func (client *baseClient) GetDel(key string) (string, error) {

 	return handleStringOrNullResponseWithFree(result), nil
 }
+
+func (client *baseClient) Ping(message string) (string, error) {


Suggest having separate Methods: Ping & PingWithMessage as in Java impl

avifenesh · 2024-10-01T20:31:28Z

Something went wrong with the CI, please have a look and fix 🙏

Yury-Fridlyand

I propose to implement a cluster client as a next task, so you can resolve a cases where a command have different implementations and docs in cluster and standalone clients. As less such commands you have at that moment, as easier that would be.

Yury-Fridlyand · 2024-10-09T14:56:44Z

go/api/commands.go

+//
+// See [valkey.io] for details.
+type ConnectionManagementCommands interface {
+	// Ping sends the PING command to the server.


Suggested change

// Ping sends the PING command to the server.

// Pings the server.

Or copy from docs from another client

Yury-Fridlyand · 2024-10-09T14:57:01Z

go/api/commands.go

+	//
+	// [valkey.io]: https://valkey.io/commands/lcs/
+	LCS(key1 string, key2 string) (Result[string], error)
+	//  If key does not exist, returns a [api.NilResult[string]] (api.CreateNilStringResult()).


Is it a part of docstrings or a code comment?

Yury-Fridlyand · 2024-10-09T14:58:54Z

go/api/commands.go

+	//
+	//[valkey.io]: https://valkey.io/commands/ping/
+	Ping() (string, error)
+	PingWithMessage(message string) (string, error)


Are golang docstrings propagated to all API in such case? If no or you'r not sure - please duplicate docs for PingWithMessage.

You can check it with a doc generator.

Signed-off-by: MikeMwita <[email protected]>

janhavigupta007 · 2024-10-10T04:08:38Z

go/api/commands.go

+	//  If an argument is provided, returns the argument.
+	//
+	// For example:
+	//  result, err := client.Ping("Hello")


The Ping method doesn't receive an argument here.

janhavigupta007 · 2024-10-10T04:09:22Z

go/api/commands.go

+	// If no argument is provided, returns "PONG".
+	//
+	// Return value:
+	//  If no argument is provided, returns "PONG".


Is it possible to not provide an argument in this method?

janhavigupta007 · 2024-10-10T04:10:50Z

go/api/base_client.go

@@ -512,3 +512,31 @@ func (client *baseClient) RPush(key string, elements []string) (Result[int64], e

 	return handleLongResponse(result)
 }
+
+func (client *baseClient) Ping() (string, error) {


I think we should return Result[string] from all the methods to keep it consistent and pass the nil string info if an error has occurred. Can you please address this in another PR?

MikeMwita requested a review from a team as a code owner September 10, 2024 20:25

MikeMwita self-assigned this Sep 10, 2024

Yury-Fridlyand reviewed Sep 10, 2024

View reviewed changes

MikeMwita force-pushed the implement-ping-command branch from f4b1d63 to cedbffb Compare September 12, 2024 19:33

avifenesh requested a review from eifrah-aws September 13, 2024 14:54

eifrah-aws approved these changes Sep 15, 2024

View reviewed changes

asafpamzn added the go golang wrapper label Sep 15, 2024

MikeMwita linked an issue Sep 15, 2024 that may be closed by this pull request

GO: implement PING command #2243

Closed

2 tasks

janhavigupta007 reviewed Sep 16, 2024

View reviewed changes

ravisi21 reviewed Sep 16, 2024

View reviewed changes

MikeMwita force-pushed the implement-ping-command branch from cedbffb to 5e31c97 Compare September 30, 2024 20:02

MikeMwita force-pushed the implement-ping-command branch 6 times, most recently from 00a14bc to 8e050d1 Compare October 9, 2024 13:02

Yury-Fridlyand approved these changes Oct 9, 2024

View reviewed changes

MikeMwita force-pushed the implement-ping-command branch from 8e050d1 to 338b8e2 Compare October 9, 2024 18:38

Go: Implementing PING command

0abbd0d

Signed-off-by: MikeMwita <[email protected]>

MikeMwita force-pushed the implement-ping-command branch from 338b8e2 to 0abbd0d Compare October 9, 2024 19:02

MikeMwita merged commit ff97ca5 into valkey-io:main Oct 9, 2024
13 checks passed

janhavigupta007 reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: Implementing PING command #2264

Go: Implementing PING command #2264

MikeMwita commented Sep 10, 2024

Yury-Fridlyand left a comment

Yury-Fridlyand Sep 10, 2024

MikeMwita Sep 12, 2024

janhavigupta007 Sep 16, 2024

Yury-Fridlyand Sep 16, 2024

janhavigupta007 Sep 16, 2024

MikeMwita Oct 9, 2024

janhavigupta007 Sep 16, 2024

ravisi21 Sep 16, 2024

avifenesh commented Oct 1, 2024

Yury-Fridlyand left a comment

Yury-Fridlyand Oct 9, 2024

Yury-Fridlyand Oct 9, 2024

Yury-Fridlyand Oct 9, 2024

Yury-Fridlyand Oct 9, 2024

janhavigupta007 Oct 10, 2024

janhavigupta007 Oct 10, 2024

janhavigupta007 Oct 10, 2024

	async def ping(
	self, message: Optional[TEncodable] = None, route: Optional[Route] = None
	) -> bytes:
	"""
	Ping the server.

	// Ping sends the PING command to the server.
	// Pings the server.

Go: Implementing PING command #2264

Go: Implementing PING command #2264

Conversation

MikeMwita commented Sep 10, 2024

Yury-Fridlyand 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

avifenesh commented Oct 1, 2024

Yury-Fridlyand 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