Copilot commented on code in PR #1451:
URL: https://github.com/apache/pulsar-client-go/pull/1451#discussion_r2625320194
##########
pulsaradmin/pkg/utils/data.go:
##########
@@ -410,18 +412,20 @@ type CursorStats struct {
}
type PartitionedTopicStats struct {
- MsgRateIn float64 `json:"msgRateIn"`
- MsgRateOut float64 `json:"msgRateOut"`
- MsgThroughputIn float64
`json:"msgThroughputIn"`
- MsgThroughputOut float64
`json:"msgThroughputOut"`
- AverageMsgSize float64 `json:"averageMsgSize"`
- StorageSize int64 `json:"storageSize"`
- Publishers []PublisherStats `json:"publishers"`
- Subscriptions map[string]SubscriptionStats `json:"subscriptions"`
- Replication map[string]ReplicatorStats `json:"replication"`
- DeDuplicationStatus string
`json:"deduplicationStatus"`
- Metadata PartitionedTopicMetadata `json:"metadata"`
- Partitions map[string]TopicStats `json:"partitions"`
+ MsgRateIn float64 `json:"msgRateIn"`
+ MsgRateOut float64 `json:"msgRateOut"`
+ MsgThroughputIn float64
`json:"msgThroughputIn"`
+ MsgThroughputOut float64
`json:"msgThroughputOut"`
+ AverageMsgSize float64
`json:"averageMsgSize"`
+ StorageSize int64 `json:"storageSize"`
+ Publishers []PublisherStats `json:"publishers"`
+ Subscriptions map[string]SubscriptionStats
`json:"subscriptions"`
+ Replication map[string]ReplicatorStats `json:"replication"`
+ DeDuplicationStatus string
`json:"deduplicationStatus"`
+ Metadata PartitionedTopicMetadata `json:"metadata"`
+ Partitions map[string]TopicStats `json:"partitions"`
+ TopicCreationTimeStamp int64
`json:"topicCreationTimeStamp,omitempty"`
Review Comment:
The field name uses "TimeStamp" (capital S) which is inconsistent with Go
naming conventions and with other timestamp fields in this codebase. All other
timestamp fields use "Timestamp" with lowercase 's' (e.g., LastPublishTimestamp
on line 428, LastConsumedTimestamp, LastAckedTimestamp, etc.). Consider
renaming this field to "TopicCreationTimestamp" for consistency, unless the
JSON tag "topicCreationTimeStamp" is dictated by the Pulsar broker API (in
which case only the Go field name should change).
```suggestion
TopicCreationTimestamp int64
`json:"topicCreationTimeStamp,omitempty"`
```
##########
pulsaradmin/pkg/utils/data.go:
##########
@@ -229,19 +229,21 @@ type NamespacesData struct {
}
type TopicStats struct {
- BacklogSize int64 `json:"backlogSize"`
- MsgCounterIn int64 `json:"msgInCounter"`
- MsgCounterOut int64 `json:"msgOutCounter"`
- MsgRateIn float64 `json:"msgRateIn"`
- MsgRateOut float64 `json:"msgRateOut"`
- MsgThroughputIn float64
`json:"msgThroughputIn"`
- MsgThroughputOut float64
`json:"msgThroughputOut"`
- AverageMsgSize float64 `json:"averageMsgSize"`
- StorageSize int64 `json:"storageSize"`
- Publishers []PublisherStats `json:"publishers"`
- Subscriptions map[string]SubscriptionStats `json:"subscriptions"`
- Replication map[string]ReplicatorStats `json:"replication"`
- DeDuplicationStatus string
`json:"deduplicationStatus"`
+ BacklogSize int64 `json:"backlogSize"`
+ MsgCounterIn int64
`json:"msgInCounter"`
+ MsgCounterOut int64
`json:"msgOutCounter"`
+ MsgRateIn float64 `json:"msgRateIn"`
+ MsgRateOut float64 `json:"msgRateOut"`
+ MsgThroughputIn float64
`json:"msgThroughputIn"`
+ MsgThroughputOut float64
`json:"msgThroughputOut"`
+ AverageMsgSize float64
`json:"averageMsgSize"`
+ StorageSize int64 `json:"storageSize"`
+ Publishers []PublisherStats `json:"publishers"`
+ Subscriptions map[string]SubscriptionStats
`json:"subscriptions"`
+ Replication map[string]ReplicatorStats `json:"replication"`
+ DeDuplicationStatus string
`json:"deduplicationStatus"`
+ TopicCreationTimeStamp int64
`json:"topicCreationTimeStamp,omitempty"`
Review Comment:
The field name uses "TimeStamp" (capital S) which is inconsistent with Go
naming conventions and with other timestamp fields in this codebase. All other
timestamp fields use "Timestamp" with lowercase 's' (e.g., LastPublishTimestamp
on line 246, LastConsumedTimestamp, LastAckedTimestamp, etc.). Consider
renaming this field to "TopicCreationTimestamp" for consistency, unless the
JSON tag "topicCreationTimeStamp" is dictated by the Pulsar broker API (in
which case only the Go field name should change).
```suggestion
TopicCreationTimestamp int64
`json:"topicCreationTimeStamp,omitempty"`
```
##########
pulsaradmin/pkg/admin/topic_test.go:
##########
@@ -1554,3 +1554,103 @@ func TestTopics_PublishRate(t *testing.T) {
100*time.Millisecond,
)
}
+
+func TestTopicStatsTimestamps(t *testing.T) {
+ topic :=
fmt.Sprintf("persistent://public/default/test-topic-stats-timestamps-%d",
time.Now().Nanosecond())
+ cfg := &config.Config{}
+ admin, err := New(cfg)
+ assert.NoError(t, err)
+
+ topicName, err := utils.GetTopicName(topic)
+ assert.NoError(t, err)
+
+ // Create topic
+ err = admin.Topics().Create(*topicName, 0)
+ assert.NoError(t, err)
+ defer admin.Topics().Delete(*topicName, true, true)
+
+ // Get stats and verify CreationTimestamp
+ stats, err := admin.Topics().GetStats(*topicName)
+ assert.NoError(t, err)
+ assert.Greater(t, stats.TopicCreationTimeStamp, int64(0),
"CreationTimestamp should be greater than 0")
+ // LastPublishTimestamp should be 0 before any message is published
+ assert.Equal(t, int64(0), stats.LastPublishTimestamp,
"LastPublishTimestamp should be 0 initially")
+
+ // Publish a message
+ client, err := pulsar.NewClient(pulsar.ClientOptions{
+ URL: lookupURL,
+ })
+ assert.NoError(t, err)
+ defer client.Close()
+
+ producer, err := client.CreateProducer(pulsar.ProducerOptions{
+ Topic: topic,
+ })
+ assert.NoError(t, err)
+ defer producer.Close()
+
+ ctx := context.Background()
+ _, err = producer.Send(ctx, &pulsar.ProducerMessage{
+ Payload: []byte("test-message"),
+ })
+ assert.NoError(t, err)
+
+ // Wait for stats to update (stats are updated asynchronously in broker)
+ assert.Eventually(t, func() bool {
+ s, err := admin.Topics().GetStats(*topicName)
+ if err != nil {
+ return false
+ }
+ return s.LastPublishTimestamp > 0
+ }, 10*time.Second, 500*time.Millisecond, "LastPublishTimestamp should
update after publishing")
+}
+
+func TestPartitionedTopicStatsTimestamps(t *testing.T) {
+ topic :=
fmt.Sprintf("persistent://public/default/test-partitioned-topic-stats-timestamps-%d",
time.Now().Nanosecond())
+ cfg := &config.Config{}
+ admin, err := New(cfg)
+ assert.NoError(t, err)
+
+ topicName, err := utils.GetTopicName(topic)
+ assert.NoError(t, err)
+
+ // Create partitioned topic
+ err = admin.Topics().Create(*topicName, 2)
+ assert.NoError(t, err)
+ defer admin.Topics().Delete(*topicName, true, true)
+
+ // Get partitioned stats and verify CreationTimestamp
+ stats, err := admin.Topics().GetPartitionedStats(*topicName, true)
+ assert.NoError(t, err)
+ assert.Greater(t, stats.TopicCreationTimeStamp, int64(0),
"CreationTimestamp should be greater than 0")
+ assert.Equal(t, int64(0), stats.LastPublishTimestamp,
"LastPublishTimestamp should be 0 initially")
+
+ // Publish a message
+ client, err := pulsar.NewClient(pulsar.ClientOptions{
+ URL: lookupURL,
+ })
+ assert.NoError(t, err)
+ defer client.Close()
+
+ producer, err := client.CreateProducer(pulsar.ProducerOptions{
+ Topic: topic,
+ })
+ assert.NoError(t, err)
+ defer producer.Close()
+
+ ctx := context.Background()
+ _, err = producer.Send(ctx, &pulsar.ProducerMessage{
+ Payload: []byte("test-message"),
+ })
+ assert.NoError(t, err)
+
+ // Wait for stats to update
+ assert.Eventually(t, func() bool {
+ s, err := admin.Topics().GetPartitionedStats(*topicName, true)
+ if err != nil {
+ return false
+ }
+ // Partitioned stats LastPublishTimestamp is usually an
aggregation or max of partitions
+ return s.LastPublishTimestamp > 0
+ }, 10*time.Second, 500*time.Millisecond, "LastPublishTimestamp should
update after publishing")
+}
Review Comment:
These two test functions (TestTopicStatsTimestamps and
TestPartitionedTopicStatsTimestamps) have significant code duplication.
Consider extracting the common test logic into a helper function that accepts a
parameter to determine whether to test a regular or partitioned topic. This
would reduce duplication, improve maintainability, and ensure both tests remain
consistent. For example, create a helper like testStatsTimestamps(t *testing.T,
partitions int) that both tests can call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
