Admin client methods that return pointers lead to ambiguous behavior for unset vs `0` values

[klevy-toast]

Expected behavior

Very similar to #1429 , but regarding the methods that return pointers. If the client call returns an empty response (to signify that the value is not set), the struct that is returned will still have 0 for every field and be indistinguishable from a real override with 0 values.

I am not sure what the correct behavior should be.

• It could be similar to Confusing handling of empty config values in admin client #1429, where we initialize the structs with a -1 sentinel value that is shown, but I'm not sure that's appropriate in all cases.
• We could return nil instead - like how the java client returns null in these cases - but that will require much more nil checking for any user.
  I am open to ideas on the right approach

Actual behavior

Empty response is unmarshalled as 0

Steps to reproduce

Appears to apply to these 14 methods:

Topic Admin Methods (10 methods):

1. GetPersistence - returns *utils.PersistenceData
2. GetDelayedDelivery - returns *utils.DelayedDeliveryData
3. GetDispatchRate - returns *utils.DispatchRateData
4. GetPublishRate - returns *utils.PublishRateData
5. GetRetention - returns *utils.RetentionPolicies
6. GetSubscribeRate - returns *utils.SubscribeRate
7. GetSubscriptionDispatchRate - returns *utils.DispatchRateData
8. GetReplicatorDispatchRate - returns *utils.DispatchRateData
9. GetAutoSubscriptionCreation - returns *utils.AutoSubscriptionCreationOverride
10. GetOffloadPolicies - returns *utils.OffloadPolicies

Namespace Admin Methods (4 methods):

1. GetRetention - returns *utils.RetentionPolicies
2. GetTopicAutoCreation - returns *utils.TopicAutoCreationConfig
3. GetBookieAffinityGroup - returns *utils.BookieAffinityGroupData
4. GetPersistence - returns *utils.PersistencePolicies

Example reproduction for topic GetPublishRate:

package main

import (
	"fmt"
	"log"
	"time"

	"github.com/apache/pulsar-client-go/pulsaradmin"
	"github.com/apache/pulsar-client-go/pulsaradmin/pkg/utils"
)

func printRate(label string, rate *utils.PublishRateData, err error) {
	fmt.Printf("%s: ", label)
	if err != nil {
		fmt.Printf("ERROR: %v\n", err)
	} else if rate == nil {
		fmt.Println("nil ✓")
	} else {
		fmt.Printf("%+v\n", *rate)
	}
}

func main() {
	cfg := &pulsaradmin.Config{WebServiceURL: "http://localhost:8080"}
	admin, err := pulsaradmin.NewClient(cfg)
	if err != nil {
		log.Fatal(err)
	}

	topicName, _ := utils.GetTopicName(fmt.Sprintf("persistent://public/default/test"))
	fmt.Printf("Topic: %s\n\n", topicName.String())

	// Create topic
	if err := admin.Topics().Create(*topicName, 0); err != nil {
		fmt.Printf("Create failed (topic may already exist): %v\n", err)
	}
	defer admin.Topics().Delete(*topicName, true, true)

	// 1. Get when not configured
	rate, err := admin.Topics().GetPublishRate(*topicName)
	printRate("1. Not configured  ", rate, err)

	// 2. Set to real values and get
	if err := admin.Topics().SetPublishRate(*topicName, utils.PublishRateData{
		PublishThrottlingRateInMsg:  100,
		PublishThrottlingRateInByte: 1024 * 1024,
	}); err != nil {
		fmt.Printf("SetPublishRate failed: %v\n", err)
	}
	time.Sleep(100 * time.Millisecond)
	rate, err = admin.Topics().GetPublishRate(*topicName)
	printRate("2. Set to 100/1MB ", rate, err)

	// 3. Set to zeros (unlimited) and get
	if err := admin.Topics().SetPublishRate(*topicName, utils.PublishRateData{
		PublishThrottlingRateInMsg:  0,
		PublishThrottlingRateInByte: 0,
	}); err != nil {
		fmt.Printf("SetPublishRate to zero failed: %v\n", err)
	}
	time.Sleep(100 * time.Millisecond)
	rate, err = admin.Topics().GetPublishRate(*topicName)
	printRate("3. Set to 0/0     ", rate, err)

	// 4. Remove and get
	if err := admin.Topics().RemovePublishRate(*topicName); err != nil {
		fmt.Printf("RemovePublishRate failed: %v\n", err)
	}
	time.Sleep(100 * time.Millisecond)
	rate, err = admin.Topics().GetPublishRate(*topicName)
	printRate("4. After removal  ", rate, err)
}

System configuration

Pulsar version: 4.x, but applies to all

[klevy-toast]

@RobertIndie do you mind providing input onto the preferred approach here?

[RobertIndie]

@klevy-toast Good finding. I think it would be better to return nil for unset values to maintain consistency with the Java admin client. We could also add documentation for each change.

[klevy-toast]

@RobertIndie Thanks for the input -- I've opened a patch here: #1433