[improve][pip] PIP-303: Add optional parameters for getPartitionedStats#21228
Conversation
BewareMyPower
left a comment
There was a problem hiding this comment.
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 @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? |
|
I think we can package all parameters other than default PartitionedTopicStats getPartitionedStats(String topic, boolean perPartition) throws PulsarAdminException {
return getPartitionedStats(topic, perPartition, false, false, false);
} |
|
Great, I'll create a class @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: If there are no other issues, I'll update the PIP docuement. |
|
The pr had no activity for 30 days, mark with Stale label. |
|
@BewareMyPower I have updated the PIP, please take a look. |
BewareMyPower
left a comment
There was a problem hiding this comment.
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. |
| TopicStatsImpl getStats(boolean getPreciseBacklog, boolean subscriptionBacklogSize, | ||
| boolean getEarliestTimeInBacklog, boolean excludePublishers, boolean excludeConsumers); | ||
|
|
||
| CompletableFuture<? extends TopicStatsImpl> asyncGetStats(boolean getPreciseBacklog, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good suggestion, done.
nodece
left a comment
There was a problem hiding this comment.
I left a suggestion, can you confirm?
LGTM
Releted issue: #21200
Documentation
docdoc-requireddoc-not-neededdoc-complete