Skip to content

[improve][pip] PIP-303: Add optional parameters for getPartitionedStats#21228

Merged
Technoboy- merged 8 commits into
apache:masterfrom
crossoverJie:pip-303
Nov 20, 2023
Merged

[improve][pip] PIP-303: Add optional parameters for getPartitionedStats#21228
Technoboy- merged 8 commits into
apache:masterfrom
crossoverJie:pip-303

Conversation

@crossoverJie

Copy link
Copy Markdown
Member

Releted issue: #21200

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions Bot added type/PIP doc-not-needed Your PR changes do not impact docs labels Sep 21, 2023

@BewareMyPower BewareMyPower left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

More overloads will be added in this way. I'm wondering if we can just pass a XXXOption to replace so many parameters?

@crossoverJie

Copy link
Copy Markdown
Member Author

More overloads will be added in this way. I'm wondering if we can just pass a XXXOption to replace so many parameters?

Good advice, I'll use GetPartitionedStatsOptions instead of exclusivePublishers/exclusiveSubscriptions.

@Data
@Builder
public class GetPartitionedStatsOptions {
    /**
     * Set to true to get precise backlog, Otherwise get imprecise backlog.
     */
    private final boolean getPreciseBacklog;

    /**
     * Whether to get backlog size for each subscription.
     */
    private final boolean subscriptionBacklogSize;

    /**
     * Whether to get the earliest time in backlog.
     */
    private final boolean getEarliestTimeInBacklog;
}

CompletableFuture<PartitionedTopicStats> getPartitionedStatsAsync(
        String topic, boolean perPartition, boolean getPreciseBacklog, boolean subscriptionBacklogSize,
        boolean getEarliestTimeInBacklog, GetPartitionedStatsOptions getPartitionedStatsOptions);

Any other suggestions?

@BewareMyPower

Copy link
Copy Markdown
Contributor

I think we can package all parameters other than topic and perPartition, which are required. You can see the following overload:

    default PartitionedTopicStats getPartitionedStats(String topic, boolean perPartition) throws PulsarAdminException {
        return getPartitionedStats(topic, perPartition, false, false, false);
    }

@crossoverJie

Copy link
Copy Markdown
Member Author

Great, I'll create a class GetPartitionedStatsOptions:

@Data
@Builder
public class GetPartitionedStatsOptions {
    /**
     * Set to true to get precise backlog, Otherwise get imprecise backlog.
     */
    private final boolean getPreciseBacklog;

    /**
     * Whether to get backlog size for each subscription.
     */
    private final boolean subscriptionBacklogSize;

    /**
     * Whether to get the earliest time in backlog.
     */
    private final boolean getEarliestTimeInBacklog;

    /**
     * Whether to exclusive publishers.
     */
    private final boolean exclusivePublishers;

    /**
     * Whether to exclusive subscriptions.
     */
    private final boolean exclusiveSubscriptions;
}

Refer to this API:

public class GetStatsOptions {
/**
* Set to true to get precise backlog, Otherwise get imprecise backlog.
*/
private final boolean getPreciseBacklog;
/**
* Whether to get backlog size for each subscription.
*/
private final boolean subscriptionBacklogSize;
/**
* Whether to get the earliest time in backlog.
*/
private final boolean getEarliestTimeInBacklog;
}

TopicStats getStats(String topic, GetStatsOptions getStatsOptions) throws PulsarAdminException;
default TopicStats getStats(String topic, boolean getPreciseBacklog, boolean subscriptionBacklogSize,
boolean getEarliestTimeInBacklog) throws PulsarAdminException {
GetStatsOptions getStatsOptions =
new GetStatsOptions(getPreciseBacklog, subscriptionBacklogSize, getEarliestTimeInBacklog);
return getStats(topic, getStatsOptions);
}

If there are no other issues, I'll update the PIP docuement.

@github-actions

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions Bot added the Stale label Oct 27, 2023
@crossoverJie

Copy link
Copy Markdown
Member Author

@BewareMyPower I have updated the PIP, please take a look.

Comment thread pip/pip-303.md Outdated
Comment thread pip/pip-303.md Outdated
@github-actions github-actions Bot removed the Stale label Nov 9, 2023

@BewareMyPower BewareMyPower left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall LGTM.

I just think the name would better be "excludeXxxx" rather than "exclusiveXxx" because the logic is excluding the publishers or consumers from the stats, not including "exclusive" publishers or consumers.

@crossoverJie

Copy link
Copy Markdown
Member Author

Overall LGTM.

I just think the name would better be "excludeXxxx" rather than "exclusiveXxx" because the logic is excluding the publishers or consumers from the stats, not including "exclusive" publishers or consumers.

This was my clerical error, done.

@Technoboy- Technoboy- added this to the 3.2.0 milestone Nov 13, 2023
Comment thread pip/pip-303.md Outdated
TopicStatsImpl getStats(boolean getPreciseBacklog, boolean subscriptionBacklogSize,
boolean getEarliestTimeInBacklog, boolean excludePublishers, boolean excludeConsumers);

CompletableFuture<? extends TopicStatsImpl> asyncGetStats(boolean getPreciseBacklog,

@nodece nodece Nov 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, we only need to add an async method to avoid blocking the thread.

For the method arguments, we can also use the above design that moving these arguments to a class.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good suggestion, done.

@nodece nodece left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left a suggestion, can you confirm?

LGTM

@Technoboy- Technoboy- merged commit 9fbb92a into apache:master Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs type/PIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants