Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MsgDropRate missing from NonPersistentTopics stats output. #11119

Merged

Conversation

MarvinCai
Copy link
Contributor

@MarvinCai MarvinCai commented Jun 27, 2021

Fixes ##10495

Motivation

MsgDropRate info is missing after NonPersistentTopics admin api merged with Topics admin api. This PR is trying to fix this.

Modifications

Seems due to API merging, data is not properly deserialized in admin client.
And also due to the added TopicsStats interface, the field hiding causing weird behavior with Jackson so fields in NonPersistentTopicStatsImpl intended to hide superclass' fields are not shown in output.

Fixing by not using same field name to hide superclass fields and use @JsonIgnore to hide them from output. And add new fields to store subscription/publisher/replicator info for NonPersistentTopic.
This does change the output name of those info, but it only changed in cli output, for admin client the old getSubscriptions/getSubscriptions/getReplication will still work.

{
  "msgRateIn" : 0.0,
  "msgThroughputIn" : 0.0,
  "msgRateOut" : 0.0,
  "msgThroughputOut" : 0.0,
  "bytesInCounter" : 0,
  "msgInCounter" : 0,
  "bytesOutCounter" : 0,
  "msgOutCounter" : 0,
  "averageMsgSize" : 0.0,
  "msgChunkPublished" : false,
  "storageSize" : 0,
  "backlogSize" : 0,
  "offloadedStorageSize" : 0,
  "lastOffloadLedgerId" : 0,
  "lastOffloadSuccessTimeStamp" : 0,
  "lastOffloadFailureTimeStamp" : 0,
  "waitingPublishers" : 0,
  "nonContiguousDeletedMessagesRanges" : 0,
  "nonContiguousDeletedMessagesRangesSerializedSize" : 0,
  "msgDropRate" : 0.0,
  "publishers" : [ ],
  "subscriptions" : {
    "my-sub" : {
      "msgRateOut" : 0.0,
      "msgThroughputOut" : 0.0,
      "bytesOutCounter" : 0,
      "msgOutCounter" : 0,
      "msgRateRedeliver" : 0.0,
      "chunkedMessageRate" : 0,
      "msgBacklog" : 0,
      "backlogSize" : 0,
      "msgBacklogNoDelayed" : 0,
      "blockedSubscriptionOnUnackedMsgs" : false,
      "msgDelayed" : 0,
      "unackedMessages" : 0,
      "type" : "Exclusive",
      "msgRateExpired" : 0.0,
      "totalMsgExpired" : 0,
      "lastExpireTimestamp" : 0,
      "lastConsumedFlowTimestamp" : 0,
      "lastConsumedTimestamp" : 0,
      "lastAckedTimestamp" : 0,
      "lastMarkDeleteAdvancedTimestamp" : 0,
      "consumers" : [ {
        "msgRateOut" : 0.0,
        "msgThroughputOut" : 0.0,
        "bytesOutCounter" : 0,
        "msgOutCounter" : 0,
        "msgRateRedeliver" : 0.0,
        "chunkedMessageRate" : 0.0,
        "consumerName" : "bfddf",
        "availablePermits" : 1000,
        "unackedMessages" : 0,
        "avgMessagesPerEntry" : 1000,
        "blockedConsumerOnUnackedMsgs" : false,
        "lastAckedTimestamp" : 0,
        "lastConsumedTimestamp" : 0,
        "metadata" : { },
        "address" : "/127.0.0.1:61295",
        "connectedSince" : "2021-08-08T13:12:51.218694+08:00",
        "clientVersion" : "2.9.0-SNAPSHOT"
      } ],
      "isDurable" : false,
      "isReplicated" : false,
      "consumersAfterMarkDeletePosition" : { },
      "nonContiguousDeletedMessagesRanges" : 0,
      "nonContiguousDeletedMessagesRangesSerializedSize" : 0,
      "msgDropRate" : 0.0,
      "durable" : false,
      "replicated" : false
    }
  },
  "replication" : { }
}

Verifying this change

  • Make sure that the change passes the CI checks.
    This change added tests and can be verified as follows:
  • Added unit test.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: yes
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

* @throws PulsarAdminException
* Unexpected error
*/
NonPersistentTopicStats getStatsNonPersistent(String topic, boolean getPreciseBacklog,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NonPersistentTopicStats also implement the TopicStats, So I think we do not need to introduce a new method in the API? We can just use the getStats method, and if users want to get the msgDropRate, they can convert TopicStats to NonPersistentTopicStats directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codelipenghui
I just tried that, once the response object got deserialized as TopicStats, it's actual type will be TopicStatsImpl at client side(we set the binding at ObjectMapperFactory ref), even server send it out as NonPersistentTopicStatsImpl, and we won't be able to convert the object back to NonPersistentTopicStats or NonPersistentTopicStatsImpl.
I think we'll have to deserialize the response to the desired type when we receive it. So a new method is needed for this purpose.

@codelipenghui codelipenghui added this to the 2.9.0 milestone Jun 29, 2021
@codelipenghui codelipenghui added the type/bug The PR fixed a bug or issue reported a bug label Jun 29, 2021
@MarvinCai MarvinCai requested a review from codelipenghui July 5, 2021 05:33
@MarvinCai
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@MarvinCai
Copy link
Contributor Author

@codelipenghui addressed your comments, PTAL

@sijie
Copy link
Member

sijie commented Jul 23, 2021

@codelipenghui Can you review this?

Comment on lines +70 to +76
public List<? extends NonPersistentPublisherStats> nonPersistentPublishers;

/** Map of non-persistent subscriptions with their individual statistics. */
public Map<String, ? extends NonPersistentSubscriptionStats> nonPersistentSubscriptions;

/** Map of non-persistent replication statistics by remote cluster context. */
public Map<String, ? extends NonPersistentReplicatorStats> nonPersistentReplicators;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add @JsonAlias to make sure the name that exposed to the JSON result not changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JsonAlias seems only used during deserialization, used @JasonProperty to keep the output field name. Updated issue description.

@MarvinCai MarvinCai requested a review from codelipenghui August 8, 2021 06:38
@codelipenghui codelipenghui merged commit 0aca5f9 into apache:master Aug 9, 2021
LeBW pushed a commit to LeBW/pulsar that referenced this pull request Aug 9, 2021
…e#11119)

Fixes #apache#10495

### Motivation
MsgDropRate info is missing after NonPersistentTopics admin api merged with Topics admin api. This PR is trying to fix this.

### Modifications
Seems due to API merging, data is not properly deserialized in admin client.
And also due to the added TopicsStats interface, the field hiding causing weird behavior with Jackson so fields in NonPersistentTopicStatsImpl intended to hide superclass' fields are not shown in output.

Fixing by not using same field name to hide superclass fields and use @JsonIgnore to hide them from output. And add new fields to store subscription/publisher/replicator info for NonPersistentTopic.
This does change the output name of those info, but it only changed in cli output, for admin client the old getSubscriptions/getSubscriptions/getReplication will still work.
hangc0276 pushed a commit that referenced this pull request Aug 12, 2021
Fixes ##10495

### Motivation
MsgDropRate info is missing after NonPersistentTopics admin api merged with Topics admin api. This PR is trying to fix this.

### Modifications
Seems due to API merging, data is not properly deserialized in admin client.
And also due to the added TopicsStats interface, the field hiding causing weird behavior with Jackson so fields in NonPersistentTopicStatsImpl intended to hide superclass' fields are not shown in output.

Fixing by not using same field name to hide superclass fields and use @JsonIgnore to hide them from output. And add new fields to store subscription/publisher/replicator info for NonPersistentTopic.
This does change the output name of those info, but it only changed in cli output, for admin client the old getSubscriptions/getSubscriptions/getReplication will still work.

(cherry picked from commit 0aca5f9)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 12, 2021
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 25, 2022
…e#11119)

Fixes #apache#10495

MsgDropRate info is missing after NonPersistentTopics admin api merged with Topics admin api. This PR is trying to fix this.

Seems due to API merging, data is not properly deserialized in admin client.
And also due to the added TopicsStats interface, the field hiding causing weird behavior with Jackson so fields in NonPersistentTopicStatsImpl intended to hide superclass' fields are not shown in output.

Fixing by not using same field name to hide superclass fields and use @JsonIgnore to hide them from output. And add new fields to store subscription/publisher/replicator info for NonPersistentTopic.
This does change the output name of those info, but it only changed in cli output, for admin client the old getSubscriptions/getSubscriptions/getReplication will still work.

(cherry picked from commit 0aca5f9)
(cherry picked from commit 8ce9e2d)
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…e#11119)

Fixes #apache#10495

### Motivation
MsgDropRate info is missing after NonPersistentTopics admin api merged with Topics admin api. This PR is trying to fix this.

### Modifications
Seems due to API merging, data is not properly deserialized in admin client.
And also due to the added TopicsStats interface, the field hiding causing weird behavior with Jackson so fields in NonPersistentTopicStatsImpl intended to hide superclass' fields are not shown in output.

Fixing by not using same field name to hide superclass fields and use @JsonIgnore to hide them from output. And add new fields to store subscription/publisher/replicator info for NonPersistentTopic.
This does change the output name of those info, but it only changed in cli output, for admin client the old getSubscriptions/getSubscriptions/getReplication will still work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants