[GitHub] merlimat commented on a change in pull request #660: Updated download links to Apache mirrors and prepared for multiple ve?
merlimat commented on a change in pull request #660: Updated download links to Apache mirrors and prepared for multiple ve? URL: https://github.com/apache/incubator-pulsar/pull/660#discussion_r132069835 ## File path: site/_config.yml ## @@ -25,9 +25,8 @@ pulsar_repo: https://github.com/apache/incubator-pulsar/tree/master baseurl: / destination: ../generated-site/content -versions: -- 1.18 -latest: 1.18 +current_version: 1.19.0-incubating Review comment: That's correct. Well, in addition to "snapshot" the docs in the `asf-site` branch, by copying the `docs/latest` directory into `docs/v1.20.0`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #660: Updated download links to Apache mirrors and prepared for multiple ve?
rdhabalia commented on a change in pull request #660: Updated download links to Apache mirrors and prepared for multiple ve? URL: https://github.com/apache/incubator-pulsar/pull/660#discussion_r132068359 ## File path: site/_config.yml ## @@ -25,9 +25,8 @@ pulsar_repo: https://github.com/apache/incubator-pulsar/tree/master baseurl: / destination: ../generated-site/content -versions: -- 1.18 -latest: 1.18 +current_version: 1.19.0-incubating Review comment: does it mean on next new release: we only need to change this file with new version? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #660: Updated download links to Apache mirrors and prepared for multiple ve?
merlimat commented on issue #660: Updated download links to Apache mirrors and prepared for multiple ve? URL: https://github.com/apache/incubator-pulsar/pull/660#issuecomment-321120532 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat opened a new pull request #660: Updated download links to Apache mirrors and prepared for multiple ve?
merlimat opened a new pull request #660: Updated download links to Apache mirrors and prepared for multiple ve? URL: https://github.com/apache/incubator-pulsar/pull/660 ?rsions docs ### Motivation With the 1.19.0-incubating release we will be storing the artifacts on the ASF dist servers, therefore we need to update the website with the right download links. Also, added links in the top-right bar to access documentation for multiple versions. NOTE: Do not merge this PR until the artifacts are already mirrored (will take several hours). cc/ @lucperkins This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] cckellogg opened a new pull request #659: Pulsar connectors
cckellogg opened a new pull request #659: Pulsar connectors URL: https://github.com/apache/incubator-pulsar/pull/659 ### Motivation Start a framework work for building source and sink connectors. ### Modifications Added a base framework for building pulsar source and sink connectors. ### Result After this change you will be able to dump a topic to google cloud storage. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat closed pull request #632: Splitted Admin API reference into multiple pages
merlimat closed pull request #632: Splitted Admin API reference into multiple pages URL: https://github.com/apache/incubator-pulsar/pull/632 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132040863
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##
@@ -245,6 +245,19 @@ protected Policies getNamespacePolicies(String property,
String cluster, String
}
}
+protected LocalPolicies getNamespaceLocalPolicies(String property, String
cluster, String namespace) {
Review comment:
Though BundleData is different. It is not configuration. It is written back,
by the brokers themselves, into the local ZK because we want each cluster to be
able to split the bundles independently.
For rate limiting, we are talking about proper configuration.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
rdhabalia commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132036537
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##
@@ -245,6 +245,19 @@ protected Policies getNamespacePolicies(String property,
String cluster, String
}
}
+protected LocalPolicies getNamespaceLocalPolicies(String property, String
cluster, String namespace) {
Review comment:
Ok. But I think we introduced local-policies to keep cluster-level policy
information such as `BundleData` and it keeps global-policies clean.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132034288
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##
@@ -1366,5 +1388,80 @@ public void markBatchMessagePublished() {
this.hasBatchMessagePublished = true;
}
+public RateLimiter getDispatchRateLimiter() {
+return dispatchRateLimiter;
+}
+
+public DispatchRateType getDispatchRateType() {
+return dispatchRateType;
+}
+
+public boolean tryDispatchPermit(long permits) {
+return permits <= 0 || dispatchRateLimiter == null
+// acquiring permits must be < configured msg-rate;
+|| dispatchRateLimiter.tryAcquire(permits);
+}
+
+public boolean hasMessageDispatchPermit() {
+return dispatchRateLimiter == null ||
dispatchRateLimiter.getAvailablePermits() > 0;
+}
+
+private void updateDispatchRate(LocalZooKeeperCacheService localZKCache) {
+final String path = joinPath(LOCAL_POLICIES_ROOT,
DestinationName.get(this.topic).getNamespace());
+try {
+Optional policies =
localZKCache.policiesCache().getAsync(path).get(cacheTimeOutInSec,
+SECONDS);
+if (policies.isPresent() && policies.get().dispatchRate != null
+&& policies.get().dispatchRate.topicDispatchRate > 0) {
+updateDispatchRate(policies.get().dispatchRate);
+log.info("[{}] configured message-dispatch rate configured at
policy {}}", this.topic,
+policies.get().dispatchRate);
+return;
+}
+} catch (Exception e) {
+log.warn("Failed to get message-rate for {}", this.topic, e);
+}
+DispatchRate dispatchRate =
brokerService.pulsar().getConfiguration().getDispatchRatePerTopicInMsg() > 0
Review comment:
It would require 2 rate limiters only if both limits are set. Otherwise one
of them, or both, will be null.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132033540
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##
@@ -245,6 +245,19 @@ protected Policies getNamespacePolicies(String property,
String cluster, String
}
}
+protected LocalPolicies getNamespaceLocalPolicies(String property, String
cluster, String namespace) {
Review comment:
That's fine, but it can be quite confusing for a user given that other
methods are changing the settings globally.
I think it could either be explicitely set for a particular cluster, or for
all the clusters. In order to clarify the scope. Eg:
```json
{
"rateLimit" : {
"dispatchRatePerTopicInMsg" : 1000.0,
"dispatchRatePerTopicInBytes" : 100.0,
"clusters" : {
"small-cluster" : {
"dispatchRatePerTopicInMsg" : 10.0,
"dispatchRatePerTopicInBytes" : 1000.0,
},
"large-cluster" : {
"dispatchRatePerTopicInMsg" : 100.0,
"dispatchRatePerTopicInBytes" : 10.0,
}
}
}
}
```
The advantage is that you can also see all the limit in a single place.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
rdhabalia commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132031203
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##
@@ -1366,5 +1388,80 @@ public void markBatchMessagePublished() {
this.hasBatchMessagePublished = true;
}
+public RateLimiter getDispatchRateLimiter() {
+return dispatchRateLimiter;
+}
+
+public DispatchRateType getDispatchRateType() {
+return dispatchRateType;
+}
+
+public boolean tryDispatchPermit(long permits) {
+return permits <= 0 || dispatchRateLimiter == null
+// acquiring permits must be < configured msg-rate;
+|| dispatchRateLimiter.tryAcquire(permits);
+}
+
+public boolean hasMessageDispatchPermit() {
+return dispatchRateLimiter == null ||
dispatchRateLimiter.getAvailablePermits() > 0;
+}
+
+private void updateDispatchRate(LocalZooKeeperCacheService localZKCache) {
+final String path = joinPath(LOCAL_POLICIES_ROOT,
DestinationName.get(this.topic).getNamespace());
+try {
+Optional policies =
localZKCache.policiesCache().getAsync(path).get(cacheTimeOutInSec,
+SECONDS);
+if (policies.isPresent() && policies.get().dispatchRate != null
+&& policies.get().dispatchRate.topicDispatchRate > 0) {
+updateDispatchRate(policies.get().dispatchRate);
+log.info("[{}] configured message-dispatch rate configured at
policy {}}", this.topic,
+policies.get().dispatchRate);
+return;
+}
+} catch (Exception e) {
+log.warn("Failed to get message-rate for {}", this.topic, e);
+}
+DispatchRate dispatchRate =
brokerService.pulsar().getConfiguration().getDispatchRatePerTopicInMsg() > 0
Review comment:
> Actually, that could just be done inside DispatchRate class
Actually, using admin-api, we always define
`DispatchRateType=msgRate/byteRate` and throttling-value in `DispatchRate`
class.
However, for dynamic configuration we can't store complex-type so, created
two variables `dispatchRatePerTopicInMsg` and `dispatchRatePerTopicInByte`,
therefore, we need this logic when we have value set for both configuration.
> Ideally, if both limits are set, both of them should be applied.
This will require 2 `RateLimiter` objects for every topic which we use
across all the subscriptions. Do you think considering both limits would be
useful?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132027792
##
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/util/RateLimiter.java
##
@@ -0,0 +1,175 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util;
+
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ *
+ * A Rate Limiter that distributes permits at a configurable rate. Each {@link
#acquire()} blocks if necessary until a
+ * permit is available, and then takes it. Each {@link #tryAcquire()} tries to
acquire permits from available permits,
+ * it returns true if it succeed else returns false. Rate limiter release
configured permits at every configured rate
+ * time, so, on next ticket new fresh permits will be available.
+ *
+ * For example: if RateLimiter is configured to release 10 permits at every 1
second then RateLimiter will allow to
+ * acquire 10 permits at any time with in that 1 second.
+ *
+ */
+public class RateLimiter {
Review comment:
Ok, I found out the gist with the explanation and it makes sense. Can you
include the full explanation here as well? Also, would it make sense to have
the `1sec` period to be configurable as well? Eg: only rate limit if it's
exceeding the rate for 10 seconds instead of 1.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
rdhabalia commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132026790
##
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/util/RateLimiter.java
##
@@ -0,0 +1,175 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util;
+
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ *
+ * A Rate Limiter that distributes permits at a configurable rate. Each {@link
#acquire()} blocks if necessary until a
+ * permit is available, and then takes it. Each {@link #tryAcquire()} tries to
acquire permits from available permits,
+ * it returns true if it succeed else returns false. Rate limiter release
configured permits at every configured rate
+ * time, so, on next ticket new fresh permits will be available.
+ *
+ * For example: if RateLimiter is configured to release 10 permits at every 1
second then RateLimiter will allow to
+ * acquire 10 permits at any time with in that 1 second.
+ *
+ */
+public class RateLimiter {
Review comment:
I have started with `Guava-RateLimiter` but ended up with custom one due to
2 main reasons:
(1) achieve per-second rate-limiting and (2) custom is faster compare to
guava. I have added both [Explanation
here](https://gist.github.com/rdhabalia/324519648b8a1008ef49d30c8f4a8bf2)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat closed pull request #658: Use $UID instead of $USER in docker build
merlimat closed pull request #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #658: Use $UID instead of $USER in docker build
merlimat commented on issue #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658#issuecomment-321064613 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #632: Splitted Admin API reference into multiple pages
merlimat commented on issue #632: Splitted Admin API reference into multiple pages URL: https://github.com/apache/incubator-pulsar/pull/632#issuecomment-321056047 @rdhabalia updated This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r131990002 ## File path: conf/broker.conf ## @@ -106,6 +106,14 @@ maxUnackedMessagesPerBroker=0 # limit/2 messages maxUnackedMessagesPerSubscriptionOnBrokerBlocked=0.16 +# Default number of message dispatching throttling-limit for every topic. Using a value of 0, is disabling default +# message dispatch-throttling +dispatchRatePerTopicInMsg=0 Review comment: Can we include "throttling" in the var name? Like `dispatchThrottlingRatePerTopicInMsg` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r131991850
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##
@@ -245,6 +245,19 @@ protected Policies getNamespacePolicies(String property,
String cluster, String
}
}
+protected LocalPolicies getNamespaceLocalPolicies(String property, String
cluster, String namespace) {
Review comment:
Shouldn't we configuring the throttling policies on the global zk?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132003921
##
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/DispatchRate.java
##
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.policies.data;
+
+public class DispatchRate {
+
+public long topicDispatchRate = -1;
+public DispatchRateType dispatchRateTye = DispatchRateType.messageRate;
+
+public enum DispatchRateType {
+messageRate, byteRate;
+}
+
+public DispatchRate() {
+this.topicDispatchRate = -1;
+this.dispatchRateTye = DispatchRateType.messageRate;
+}
+
+public DispatchRate(long topicMessageRate, DispatchRateType
dispatchRateTye) {
+super();
+this.topicDispatchRate = topicMessageRate;
+this.dispatchRateTye = dispatchRateTye;
+}
+
+@Override
+public int hashCode() {
+final int prime = 31;
+int result = 1;
+result = prime * result + ((dispatchRateTye == null) ? 0 :
dispatchRateTye.hashCode());
+result = prime * result + (int) (topicDispatchRate ^
(topicDispatchRate >>> 32));
+return result;
+}
+
+@Override
+public boolean equals(Object obj) {
+if (this == obj) {
+return true;
+}
+if (obj == null) {
+return false;
+}
+if (getClass() != obj.getClass()) {
+return false;
+}
+DispatchRate other = (DispatchRate) obj;
+if (dispatchRateTye != other.dispatchRateTye)
+return false;
+if (topicDispatchRate != other.topicDispatchRate)
+return false;
+return true;
+}
+
+@Override
+public String toString() {
Review comment:
```java
return Objects.toString(this);
```
?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r131992335
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/Namespaces.java
##
@@ -810,6 +811,42 @@ public void splitNamespaceBundle(@PathParam("property")
String property, @PathPa
}
}
+@PUT
Review comment:
Should this `POST` rather than `PUT`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r131996619
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##
@@ -575,5 +578,31 @@ private void clearUnAckedMsgs(Consumer consumer) {
subscription.addUnAckedMessages(-unaAckedMsgs);
}
+public static class SendMessageInfo {
Review comment:
How is this information used?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132004308
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##
@@ -1366,5 +1388,80 @@ public void markBatchMessagePublished() {
this.hasBatchMessagePublished = true;
}
+public RateLimiter getDispatchRateLimiter() {
+return dispatchRateLimiter;
+}
+
+public DispatchRateType getDispatchRateType() {
+return dispatchRateType;
+}
+
+public boolean tryDispatchPermit(long permits) {
+return permits <= 0 || dispatchRateLimiter == null
+// acquiring permits must be < configured msg-rate;
+|| dispatchRateLimiter.tryAcquire(permits);
+}
+
+public boolean hasMessageDispatchPermit() {
+return dispatchRateLimiter == null ||
dispatchRateLimiter.getAvailablePermits() > 0;
+}
+
+private void updateDispatchRate(LocalZooKeeperCacheService localZKCache) {
+final String path = joinPath(LOCAL_POLICIES_ROOT,
DestinationName.get(this.topic).getNamespace());
+try {
+Optional policies =
localZKCache.policiesCache().getAsync(path).get(cacheTimeOutInSec,
+SECONDS);
+if (policies.isPresent() && policies.get().dispatchRate != null
+&& policies.get().dispatchRate.topicDispatchRate > 0) {
+updateDispatchRate(policies.get().dispatchRate);
+log.info("[{}] configured message-dispatch rate configured at
policy {}}", this.topic,
+policies.get().dispatchRate);
+return;
+}
+} catch (Exception e) {
+log.warn("Failed to get message-rate for {}", this.topic, e);
+}
+DispatchRate dispatchRate =
brokerService.pulsar().getConfiguration().getDispatchRatePerTopicInMsg() > 0
Review comment:
Actually, that could just be done inside `DispatchRate` class
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132003620
##
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/DispatchRate.java
##
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.policies.data;
+
+public class DispatchRate {
+
+public long topicDispatchRate = -1;
+public DispatchRateType dispatchRateTye = DispatchRateType.messageRate;
+
+public enum DispatchRateType {
+messageRate, byteRate;
+}
+
+public DispatchRate() {
+this.topicDispatchRate = -1;
+this.dispatchRateTye = DispatchRateType.messageRate;
+}
+
+public DispatchRate(long topicMessageRate, DispatchRateType
dispatchRateTye) {
+super();
+this.topicDispatchRate = topicMessageRate;
+this.dispatchRateTye = dispatchRateTye;
+}
+
+@Override
+public int hashCode() {
Review comment:
Unless `hashCode()` is used in a very critical place, just use
`Objects.hashCode(var1, var2)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #634: PIP-3 : Introduce message-dispatch rate limiting
merlimat commented on a change in pull request #634: PIP-3 : Introduce
message-dispatch rate limiting
URL: https://github.com/apache/incubator-pulsar/pull/634#discussion_r132003259
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##
@@ -1366,5 +1388,80 @@ public void markBatchMessagePublished() {
this.hasBatchMessagePublished = true;
}
+public RateLimiter getDispatchRateLimiter() {
+return dispatchRateLimiter;
+}
+
+public DispatchRateType getDispatchRateType() {
+return dispatchRateType;
+}
+
+public boolean tryDispatchPermit(long permits) {
+return permits <= 0 || dispatchRateLimiter == null
+// acquiring permits must be < configured msg-rate;
+|| dispatchRateLimiter.tryAcquire(permits);
+}
+
+public boolean hasMessageDispatchPermit() {
+return dispatchRateLimiter == null ||
dispatchRateLimiter.getAvailablePermits() > 0;
+}
+
+private void updateDispatchRate(LocalZooKeeperCacheService localZKCache) {
+final String path = joinPath(LOCAL_POLICIES_ROOT,
DestinationName.get(this.topic).getNamespace());
+try {
+Optional policies =
localZKCache.policiesCache().getAsync(path).get(cacheTimeOutInSec,
+SECONDS);
+if (policies.isPresent() && policies.get().dispatchRate != null
+&& policies.get().dispatchRate.topicDispatchRate > 0) {
+updateDispatchRate(policies.get().dispatchRate);
+log.info("[{}] configured message-dispatch rate configured at
policy {}}", this.topic,
+policies.get().dispatchRate);
+return;
+}
+} catch (Exception e) {
+log.warn("Failed to get message-rate for {}", this.topic, e);
+}
+DispatchRate dispatchRate =
brokerService.pulsar().getConfiguration().getDispatchRatePerTopicInMsg() > 0
Review comment:
Ideally, if both limits are set, both of them should be applied.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on issue #658: Use $UID instead of $USER in docker build
merlimat commented on issue #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658#issuecomment-321050797 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #658: Use $UID instead of $USER in docker build
merlimat commented on issue #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658#issuecomment-321050797 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #632: Splitted Admin API reference into multiple pages
merlimat commented on a change in pull request #632: Splitted Admin API
reference into multiple pages
URL: https://github.com/apache/incubator-pulsar/pull/632#discussion_r132002230
##
File path: site/docs/latest/admin-api/permissions.md
##
@@ -1,155 +1,6 @@
-Pulsar {% popover namespaces %} are logical groupings of {% popover topics %}.
-
-Namespaces can be managed via:
-
-* The [`namespaces`](../../reference/CliTools#pulsar-admin-clusters) command
of the [`pulsar-admin`](../../reference/CliTools#pulsar-admin) tool
-* The `/admin/namespaces` endpoint of the admin [REST
API](../../reference/RestApi)
-* The `namespaces` method of the {% javadoc PulsarAdmin admin
org.apache.pulsar.client.admin.PulsarAdmin %} object in the [Java
API](../../applications/JavaClient)
-
-### Create
-
-You can create new namespaces under a given {% popover property %} and within
a Pulsar {% popover cluster %}.
-
- pulsar-admin
-
-Use the [`create`](../../reference/CliTools#pulsar-admin-namespaces-create)
subcommand and specify the namespace by name:
-
-```shell
-$ pulsar-admin namespaces create test-property/cl1/ns1
-```
-
- REST API
-
-{% endpoint PUT /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().createNamespace(namespace);
-```
-
-### Get policies
-
-You can fetch the current policies associated with a namespace at any time.
-
- pulsar-admin
-
-Use the
[`policies`](../../reference/CliTools#pulsar-admin-namespaces-policies)
subcommand and specify the namespace:
-
-```shell
-$ pulsar-admin namespaces policies test-property/cl1/ns1
-{
- "auth_policies": {
-"namespace_auth": {},
-"destination_auth": {}
- },
- "replication_clusters": [],
- "bundles_activated": true,
- "bundles": {
-"boundaries": [
- "0x",
- "0x"
-],
-"numBundles": 1
- },
- "backlog_quota_map": {},
- "persistence": null,
- "latency_stats_sample_rate": {},
- "message_ttl_in_seconds": 0,
- "retention_policies": null,
- "deleted": false
-}
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().getPolicies(namespace);
-```
-
-### List namespaces within a property
-
-You can list all namespaces within a given Pulsar {% popover property %}.
-
- pulsar-admin
-
-Use the [`list`](../../reference/CliTools#pulsar-admin-namespaces-list)
subcommand and specify the property:
-
-```shell
-$ pulsar-admin namespaces list test-property
-test-property/cl1/ns1
-test-property/cl2/ns2
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property %}
-
-[More info](../../reference/RestApi#/admin/namespaces/:property)
-
- Java
-
-```java
-admin.namespaces().getNamespaces(property);
-```
-
-### List namespaces within a cluster
-
-You can list all namespaces within a given Pulsar {% popover cluster %}.
-
- pulsar-admin
-
-Use the
[`list-cluster`](../../reference/CliTools#pulsar-admin-namespaces-list-cluster)
subcommand and specify the cluster:
-
-```shell
-$ pulsar-admin namespaces list-cluster test-property/cl1
-test-property/cl1/ns1
-test-property/cl1/ns1
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property/:cluster %}
-
-[More info](../../reference/RestApi#/admin/namespaces/:property/:cluster)
-
- Java
-
-```java
-admin.namespaces().getNamespaces(property, cluster);
-```
-
-### Delete
-
-You can delete existing namespaces from a property/cluster.
-
- pulsar-admin
-
-Use the [`delete`](../../reference/CliTools#pulsar-admin-namespaces-delete)
subcommand and specify the namespace:
-
-```shell
-$ pulsar-admin namespaces delete test-property/cl1/ns1
-```
-
- REST
-
-{% endpoint DELETE /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().deleteNamespace(namespace);
-```
+---
+title: Managing permissions
+---
Review comment:
You're right, I broke it down in the wrong place. Put all these operations
under namespace again.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #632: Splitted Admin API reference into multiple pages
merlimat commented on a change in pull request #632: Splitted Admin API
reference into multiple pages
URL: https://github.com/apache/incubator-pulsar/pull/632#discussion_r132001213
##
File path: site/docs/latest/admin-api/permissions.md
##
@@ -1,155 +1,6 @@
-Pulsar {% popover namespaces %} are logical groupings of {% popover topics %}.
-
-Namespaces can be managed via:
-
-* The [`namespaces`](../../reference/CliTools#pulsar-admin-clusters) command
of the [`pulsar-admin`](../../reference/CliTools#pulsar-admin) tool
-* The `/admin/namespaces` endpoint of the admin [REST
API](../../reference/RestApi)
-* The `namespaces` method of the {% javadoc PulsarAdmin admin
org.apache.pulsar.client.admin.PulsarAdmin %} object in the [Java
API](../../applications/JavaClient)
-
-### Create
-
-You can create new namespaces under a given {% popover property %} and within
a Pulsar {% popover cluster %}.
-
- pulsar-admin
-
-Use the [`create`](../../reference/CliTools#pulsar-admin-namespaces-create)
subcommand and specify the namespace by name:
-
-```shell
-$ pulsar-admin namespaces create test-property/cl1/ns1
-```
-
- REST API
-
-{% endpoint PUT /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().createNamespace(namespace);
-```
-
-### Get policies
-
-You can fetch the current policies associated with a namespace at any time.
-
- pulsar-admin
-
-Use the
[`policies`](../../reference/CliTools#pulsar-admin-namespaces-policies)
subcommand and specify the namespace:
-
-```shell
-$ pulsar-admin namespaces policies test-property/cl1/ns1
-{
- "auth_policies": {
-"namespace_auth": {},
-"destination_auth": {}
- },
- "replication_clusters": [],
- "bundles_activated": true,
- "bundles": {
-"boundaries": [
- "0x",
- "0x"
-],
-"numBundles": 1
- },
- "backlog_quota_map": {},
- "persistence": null,
- "latency_stats_sample_rate": {},
- "message_ttl_in_seconds": 0,
- "retention_policies": null,
- "deleted": false
-}
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().getPolicies(namespace);
-```
-
-### List namespaces within a property
-
-You can list all namespaces within a given Pulsar {% popover property %}.
-
- pulsar-admin
-
-Use the [`list`](../../reference/CliTools#pulsar-admin-namespaces-list)
subcommand and specify the property:
-
-```shell
-$ pulsar-admin namespaces list test-property
-test-property/cl1/ns1
-test-property/cl2/ns2
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property %}
-
-[More info](../../reference/RestApi#/admin/namespaces/:property)
-
- Java
-
-```java
-admin.namespaces().getNamespaces(property);
-```
-
-### List namespaces within a cluster
-
-You can list all namespaces within a given Pulsar {% popover cluster %}.
-
- pulsar-admin
-
-Use the
[`list-cluster`](../../reference/CliTools#pulsar-admin-namespaces-list-cluster)
subcommand and specify the cluster:
-
-```shell
-$ pulsar-admin namespaces list-cluster test-property/cl1
-test-property/cl1/ns1
-test-property/cl1/ns1
-```
-
- REST API
-
-{% endpoint GET /admin/namespaces/:property/:cluster %}
-
-[More info](../../reference/RestApi#/admin/namespaces/:property/:cluster)
-
- Java
-
-```java
-admin.namespaces().getNamespaces(property, cluster);
-```
-
-### Delete
-
-You can delete existing namespaces from a property/cluster.
-
- pulsar-admin
-
-Use the [`delete`](../../reference/CliTools#pulsar-admin-namespaces-delete)
subcommand and specify the namespace:
-
-```shell
-$ pulsar-admin namespaces delete test-property/cl1/ns1
-```
-
- REST
-
-{% endpoint DELETE /admin/namespaces/:property/:cluster/:namespace %}
-
-[More
info](../../reference/RestApi#/admin/namespaces/:property/:cluster/:namespace)
-
- Java
-
-```java
-admin.namespaces().deleteNamespace(namespace);
-```
+---
+title: Managing permissions
+---
## Managing permissions
Review comment:
I'll change the title below (` ## Managing permissions`) to keep it
consistent with the other admin api pages
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #632: Splitted Admin API reference into multiple pages
merlimat commented on a change in pull request #632: Splitted Admin API reference into multiple pages URL: https://github.com/apache/incubator-pulsar/pull/632#discussion_r132001068 ## File path: site/docs/latest/admin-api/partitioned-topics.md ## @@ -1,9 +1,15 @@ +--- +title: Partitioned topics Review comment: Yes, fixed This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rdhabalia commented on a change in pull request #658: Use $UID instead of $USER in docker build
rdhabalia commented on a change in pull request #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658#discussion_r131995895 ## File path: site/scripts/docker-build.sh ## @@ -35,6 +35,9 @@ echo " Build Pulsar website using image $IMAGE" docker pull $IMAGE -DOCKER_CMD="docker run --user $USER -i -v $ROOT_DIR:/pulsar $IMAGE" +CI_USER=$(id -u) +CI_GROUP=$(id -g) -$DOCKER_CMD bash -l -c 'cd /pulsar/site && rvm use . && make setup && make protobuf_doc_gen && make build' +DOCKER_CMD="docker run -i -e CI_USER=$CI_USER -e CI_GROUP=$CI_GROUP -v $ROOT_DIR:/pulsar $IMAGE" + +$DOCKER_CMD bash -l -c 'cd /pulsar/site && rvm use . && make setup && make protobuf_doc_gen && make build && chown -R $CI_USER:$CI_GROUP /pulsar/generated-site' Review comment: did you forget `'` in the end of the line? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #658: Use $UID instead of $USER in docker build
merlimat commented on issue #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658#issuecomment-321043000 @rdhabalia Please take a look. I've verified the website now build on Jenkins with this fix. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat opened a new pull request #658: Use $UID instead of $USER in docker build
merlimat opened a new pull request #658: Use $UID instead of $USER in docker build URL: https://github.com/apache/incubator-pulsar/pull/658 ### Motivation In #652, there was an attempt to fix the website build by using current user in Docker. It doesn't work in Jenkins and the (verified) solution here is to keep using "root" inside the container, but change the owner of these files after the build, so that Jenkins can read and write from outside the container. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat closed pull request #654: Do not delete inactive topics if they have retention policy
merlimat closed pull request #654: Do not delete inactive topics if they have retention policy URL: https://github.com/apache/incubator-pulsar/pull/654 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat closed pull request #641: Added missing copyright notice for Circe library
merlimat closed pull request #641: Added missing copyright notice for Circe library URL: https://github.com/apache/incubator-pulsar/pull/641 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat opened a new issue #657: Intermittent C++ test failure BatchMessageTest.testProducerConfig
merlimat opened a new issue #657: Intermittent C++ test failure BatchMessageTest.testProducerConfig URL: https://github.com/apache/incubator-pulsar/issues/657 As seen in https://builds.apache.org/job/pulsar-master/22/console ``` [17/75] BatchMessageTest.testProducerConfig (30 ms) log4cxx: Could not read configuration file [log4cxx.conf]. Note: Google Test filter = BatchMessageTest.testProducerConfig [==] Running 1 test from 1 test case. [--] Global test environment set-up. [--] 1 test from BatchMessageTest [ RUN ] BatchMessageTest.testProducerConfig [WARNING] /usr/src/gtest/src/gtest-death-test.cc:825:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test couldn't detect the number of threads. /pulsar/pulsar-client-cpp/tests/BatchMessageTest.cc:75: Failure Death test: conf.setBatchingMaxMessages(1) Result: failed to die. Error msg: [ DEATH ] [ FAILED ] BatchMessageTest.testProducerConfig (5 ms) [--] 1 test from BatchMessageTest (5 ms total) [--] Global test environment tear-down [==] 1 test from 1 test case ran. (6 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] BatchMessageTest.testProducerConfig 1 FAILED TEST [17/75] BatchMessageTest.testProducerConfig returned/aborted with exit code 1 (30 ms) ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
