[PIP-82] [pulsar-broker] Add a task to publish resource usage to a topic #10008

bharanic-dev · 2021-03-22T17:02:09Z

Motivation

PIP-82 adds support for distributed resource quota enforcement at a tenant, namespace or topic level.

Modifications

proto definition for publishing resource usage.
Use lightproto
Add a task to publish the resource usage.

Verifying this change

Added unit tests

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

If yes was chosen, please highlight the changes

Dependencies (lightproto) added to pulsar-broker

Documentation

Does this pull request introduce a new feature? (yes)
The configuration and documentation of the same will be added in a subsequent PR.

hsaputra · 2021-03-22T21:56:19Z

...oker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ResourceUsageTransportManager implements AutoCloseable {


Could you add Javadoc header comment to explain why and what this class is for?
Short summary on how this will be used or related to existing classes and module will help us to remember why this class exist or created, in the future.

Thank you. I added the javadoc comment.

bharanic-dev · 2021-03-22T22:30:34Z

/pulsabot run-failure-checks

bharanic-dev · 2021-03-23T15:26:18Z

/pulsabot run-failure-checks

bharanic-dev · 2021-03-23T18:05:40Z

/pulsabot run-failure-checks

codelipenghui

BTW, we are introduced SystemTopic mechanism in the broker, which is used by topic level policy and transactions now. I think we can improve the system topic to support non-persistent topic to achieve this purpose?

codelipenghui · 2021-03-24T09:58:53Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+            category = CATEGORY_POLICIES,
+            doc = "Topic to publish usage reports to if resourceUsagePublishToTopic is enabled."
+    )
+    private String resourceUsagePublishTopicName = "non-persistent://pulsar/system/resource-usage";


This one should be an internal topic, I think we don't need to expose it to users.

@codelipenghui thank you for the review. Prior to writing the code in this PR, I read through the SystemTopic/TopicPolicies implementation. My observation was that:

it was added to publish namespace change events to a special system topic (created under the same namespace), so every namespace will have a special topic created.

there is a SystemTopicClient wrapper that was added on top of PulsarClient(), which has methods to create reader/writer for each of the above topics. A reader/writer is created every time there is a topic policy change.

the topic schema is hard-coded to PulsarEvent with AVRO encoding.

The resource-usage feature differs in the following way:

one topic at the system level, one reader, one writer.

message schema is protobuf

periodic task to publish usages and reader callback to process usage messages.

I don't think we could have used SystemTopic for resource-usage without making any changes to SystemTopic. At a minimum I would have had to change SystemTopicClientBase implementation. In my opinion, the overlap in functionality is not large enough to make re-factoring SystemTopic and re-testing the SystemTopic feature worthwhile. So, I decided to write the simple code to create a single reader and writer (which is what the PR has).

W.r.t your other comment, the topic name is internal. The config option is exposed to the user in case the topic name conflicts with the topics that user wants to use. The user can decide that the default is OK, in which case the topic name gets decided by the implementation.

It should be internal of the resource exchange implementation? Or we can add a config such as resourceExchangeProperties=, so that this can works for other implementations.

codelipenghui · 2021-03-24T10:01:13Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+            doc = "Default policy for publishing usage reports to system topic is disabled."
+            + "This enables publishing of usage reports"
+    )
+    private boolean resourceUsagePublishToTopic = false;


Is it better to expose namespaceResourceLimiterEnabled or tenantResourceLimiterEnabled to users? the resourceUsagePublishToTopic is an internal mechanism, expose it to users is need the user to understand the details of his work

When PIP-82 was presented in the community meeting, it was suggested that the mechanism used to exchange the usage information across brokers should be configurable/pluggable (for instance, one could use a third-party DB or zookeeper or something else). This config option was added for that purpose. There will be other configuration that allows the user to configure resource-groups to manage the quotas. That implementation is still work in progress and a PR will be raised when it is ready.

Ok, if the mechanism used to exchange the usage information across brokers should be pluggable, it better to provide the implementation class name such as resourceExchangeClassName=NonPersistentTopicBasedResourceExchange? So that we can support diverse implementations?

@codelipenghui, I renamed the configuration option per your suggestion. Please take a look.

@codelipenghui, please let me know if you have any more comments. If all looks good, can you please approve, so we can get the changes merged? Thank you.

hsaputra

LGTM +1

…ageTransportClassName'.

315157973 · 2021-04-01T14:22:07Z

If we don’t use SystemTopic, we have many scenes that need to be handled separately, for example: when allowAutoTopicCreation=false, an error will be reported; Avoid being deleted as inactiveTopic, etc.
But SystemTopic has already deal with these scenarios.
So I suggest to add an implementation.

bharanic-dev · 2021-04-01T21:19:19Z

Hi @315157973 , I have already outlined some of the reasons why we decided to not use SystemTopic (please see #10008 (comment)).

If we are to use SystemTopic to exchange 'resource usage', it will require significant refactoring to SystemTopic. There is really not much overlap in functionality other than creating one producer and one consumer. In my opinion, there is nothing much to gain to refactor the SystemTopic code just to use a couple of lines of code.

With respect to allowAutoTopicCreation=false,

we don't expect this to be set to false for 'system' tenant.
even if it is, it just means that the resource usage cannot be exchanged and the default behavior will be what it is today (no quota enforcement).
And if the user chooses to have 'resource quota enforcement' while having allowAutoTopicCreation=false, the expectation is that the user creates the "resource usage publish topic" explicitly using the admin API.

[ side note: from looking at the code, I am not able figure out how SystemTopic handles the allowAutoTopicCreation= false; I'd appreciate if you can provide code pointers or documentation that explain. It would help to me to get familiarized. ]

And to your other point of inactive topic, by default the resource usage topic is non-persistent as the brokers are only interested in the current/recent usage for calculating the quotas, not historical usage. If the user chooses to use a persistent topic (via configuration) for some reason, it should be fine to assume that the user knows what they are doing and take necessary steps (they may be OK for the topic to be deleted as inactive or they may not be).

315157973 · 2021-04-02T06:33:05Z

This PR is a great feature, and we can later migrate other usage information from zk to here. So I have the above suggestions.

Using SystemTopic is just a way, not our goal. Even if we do not use SystemTopic, I suggest other methods to achieve these goals:

No errors can be reported and no manual intervention is required. For example, if there is no data after a period of time, and it is deleted, a topic needs to be created manually. So hard to maintain.
Internal topics should be a reserved field, and it is better not be perceived by users.

This is just my suggestion

Here is an explanation of your question above:

SystemTopic allows automatic creation: isAllowAutoTopicCreation(final TopicName topicName)
Although SystemTopic is persistent, it is compressed topic, and the same key will only save the last one. I didn’t think deeply about the impact if the usage information is lost
We do not have a tenant-level policy now, and may need to be at the namespace level or topic level

bharanic-dev · 2021-04-02T15:05:49Z

Are you suggesting that if 'resource quota enforcement' is enabled, it should override 'allowAutoTopicCreation=false'? My preference is not to do that, but if others agree, I can do that. Are you OK if I take it up as a separate PR?

If you strongly feel that the 'resource-usage' topic name should not be exposed in the configuration, I can hide it.

The 'resource-usage-exchange' mechanism is a broker-level mechanism. The implementation should be as simple as possible. I think we should it should not use higher level features that are use-visible, such as topic compaction. It is one of the reasons why we have chosen to not have schema validation (even though the schema is defined in protobuf).

eolivelli

Very good.
I left one suggestion
PTAL

eolivelli · 2021-04-05T17:38:20Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageConsumer.java

+     * Listener for resource usage published by other brokers
+     * @param broker name
+     * @param Resource usage object
+     */


What about 'acceptResourceUsage'?

@eolivelli , I changed the name of the method, per your suggestion. Please take a look.

@eolivelli I have incorporated your comments and the CI passed as well. Can you please take a look and approve. Thank you!

good, thanks

jerrypeng · 2021-04-05T20:31:57Z

...oker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java

+                    .batchingMaxPublishDelay(publishDelayMilliSecs, TimeUnit.MILLISECONDS)
+                    .sendTimeout(sendTimeoutSecs, TimeUnit.SECONDS)
+                    .blockIfQueueFull(false)
+                    .compressionType(SNAPPY)


Just wondering, if there a particular reason snappy was chosen as the compression algorithm?

@jerrypeng there is no particular reason. If you believe we should use some other compression algorithm, please let me know. We may also change this after testing if we find that some other algorithm is better.

jerrypeng · 2021-04-05T20:50:46Z

...oker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java

+
+        List<String> tenantList =  admin.tenants().getTenants();
+        if (!tenantList.contains(tenant)) {
+            admin.tenants().createTenant(tenant,


Every broker when attempt to run this correct? If so, I would add some retry logic for this whole block in case multiple brokers are trying to do this at the same time. In that scenario only one succeeds but others can just retry to make sure the tenant is already created.

@jerrypeng Yes, every broker will attempt to run this. I can add the retry logic, but I don't understand why we need one. One of the brokers will succeed and others will find that the tenant/namespace exist and move on. The one race condition that is not handled is if after the check for exists, the tenant gets created by some other broker, this broker might get ConflictException. I will add a try/catch to ignore this exception. However, can I please take that up in my next commit? I can create a github issue to track it.

The one race condition that is not handled is if after the check for exists, the tenant gets created by some other broker, this broker might get ConflictException

Yes that exact scenario. We have added similar logic in the worker:

https://github.com/apache/pulsar/blob/master/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/PulsarWorkerService.java#L229

To handle that exact situation. It is a lot more lightweight solution that to force a restart.

However, can I please take that up in my next commit? I can create a github issue to track it.

sure

@jerrypeng Yes, every broker will attempt to run this. I can add the retry logic, but I don't understand why we need one. One of the brokers will succeed and others will find that the tenant/namespace exist and move on. The one race condition that is not handled is if after the check for exists, the tenant gets created by some other broker, this broker might get ConflictException. I will add a try/catch to ignore this exception. However, can I please take that up in my next commit? I can create a github issue to track it.

To add try/catch is a good solution.

bharanic-dev · 2021-04-07T20:14:00Z

/pulsarbot run-failure-checks

bharanic-dev · 2021-04-07T22:45:25Z

/pulsarbot run-failure-checks

bharanic-dev · 2021-04-07T22:46:53Z

/pulsarbot run-failure-checks

bharanic-dev · 2021-04-08T14:59:38Z

/pulsarbot run-failure-checks

bharanic-dev · 2021-04-08T15:16:01Z

/pulsarbot run-failure-checks

eolivelli

LGTM

great work

eolivelli · 2021-04-09T06:15:31Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageConsumer.java

+     * Listener for resource usage published by other brokers
+     * @param broker name
+     * @param Resource usage object
+     */


good, thanks

eolivelli · 2021-04-09T06:18:08Z

IIUC @jerrypeng comment will be addresses in the next commit and he agrees with that.
I am merging now this patch

thank you @bharanic-dev

…pic (apache#10008)

bharanic-dev · 2021-04-09T16:24:26Z

@eolivelli, thank you for the review and for merging the changes to master.

…pic (apache#10008)

Hongten · 2023-02-10T03:09:50Z

...oker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceUsageTransportManager.java

+            return pulsarClient.newProducer()
+                    .topic(pulsarService.getConfig().getResourceUsageTransportPublishTopicName())
+                    .batchingMaxPublishDelay(publishDelayMilliSecs, TimeUnit.MILLISECONDS)
+                    .sendTimeout(sendTimeoutSecs, TimeUnit.SECONDS)


I wonder, it seems we can hardcode 10 here, right? why do we need to use the verb sendTimeoutSecs?

Bharani Chadalavada added 3 commits March 22, 2021 08:48

Proto for usage reports

4581e24

Change to proto2 and use lightproto

2f14c89

Add task to publish/consume resource-usage info to pulsar topic.

d81fc2c

hsaputra reviewed Mar 22, 2021

View reviewed changes

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Mar 22, 2021

merlimat added this to the 2.8.0 milestone Mar 22, 2021

merlimat assigned bharanic-dev Mar 22, 2021

Bharani Chadalavada added 2 commits March 23, 2021 15:39

Add javadoc comment; fixed a typo bug in the code

95a9b7e

Commit the accidental omission

5b1bae0

codelipenghui reviewed Mar 24, 2021

View reviewed changes

hsaputra approved these changes Mar 25, 2021

View reviewed changes

Rename the configuration 'resourceUsagePublishToTopic' to 'resourceUs…

4e8970f

…ageTransportClassName'.

codelipenghui requested review from zymap, 315157973, hangc0276, jiazhai, merlimat, sijie, congbobo184 and gaoran10 March 31, 2021 13:58

hsaputra approved these changes Apr 2, 2021

View reviewed changes

codelipenghui approved these changes Apr 2, 2021

View reviewed changes

merlimat approved these changes Apr 5, 2021

View reviewed changes

eolivelli requested changes Apr 5, 2021

View reviewed changes

jerrypeng reviewed Apr 5, 2021

View reviewed changes

rename 'resourceUsageListener' to 'acceptResourceUsage'.

df39b95

eolivelli approved these changes Apr 9, 2021

View reviewed changes

eolivelli merged commit cb507da into apache:master Apr 9, 2021

wangjialing218 pushed a commit to wangjialing218/pulsar that referenced this pull request Apr 9, 2021

[PIP-82] [pulsar-broker] Add a task to publish resource usage to a to…

d61f5b1

…pic (apache#10008)

bharanic-dev mentioned this pull request Apr 12, 2021

[PIP-82] [pulsar-broker] incorporate review feedback #10201

Merged

1 task

ivankelly pushed a commit to ivankelly/pulsar that referenced this pull request Aug 10, 2021

[PIP-82] [pulsar-broker] Add a task to publish resource usage to a to…

dd54698

…pic (apache#10008)

Hongten reviewed Feb 10, 2023

View reviewed changes

[PIP-82] [pulsar-broker] Add a task to publish resource usage to a topic #10008

[PIP-82] [pulsar-broker] Add a task to publish resource usage to a topic #10008

Conversation

bharanic-dev commented Mar 22, 2021

Motivation

Modifications

Verifying this change

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

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharanic-dev commented Mar 22, 2021

bharanic-dev commented Mar 23, 2021

bharanic-dev commented Mar 23, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharanic-dev Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsaputra left a comment

Choose a reason for hiding this comment

315157973 commented Apr 1, 2021 • edited Loading

bharanic-dev commented Apr 1, 2021

315157973 commented Apr 2, 2021

bharanic-dev commented Apr 2, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharanic-dev commented Apr 7, 2021

bharanic-dev commented Apr 7, 2021

bharanic-dev commented Apr 7, 2021

bharanic-dev commented Apr 8, 2021

bharanic-dev commented Apr 8, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli commented Apr 9, 2021

bharanic-dev commented Apr 9, 2021

Choose a reason for hiding this comment

bharanic-dev Mar 25, 2021 •

edited

Loading

315157973 commented Apr 1, 2021 •

edited

Loading

jerrypeng Apr 5, 2021 •

edited

Loading

jerrypeng Apr 7, 2021 •

edited

Loading