You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/14 12:14:57 UTC
[GitHub] [pulsar] Shoothzj opened a new pull request #13291: Fix incompatibility of BacklogQuota
Shoothzj opened a new pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291
### Motivation
there is a change between pulsar2.7 and master
command
```
bin/pulsar admin namespaces get-backlog-quotas $tenant/$namespace
```
pulsar2.7 returns
```
destination_storage BacklogQuotaImpl(limit=102400, limitTime=-1, policy=consumer_backlog_eviction)
```
but pulsar 2.9 returns
```[root@23da8000c7c1 bin]# ./pulsar-admin namespaces get-backlog-quotas public/functions
"destination_storage BacklogQuotaImpl(limitSize=102400, limitTime=-1, policy=consumer_backlog_eviction)"
```
So we need a compat, let old REST/sdk user, can set and get `limit`
### Verifying this change
- add `org.apache.pulsar.broker.resources.NamespaceBacklogQuotaCompatibleTest` for test
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
It's a compatibility bug fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#discussion_r769313471
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BacklogQuotaImpl.java
##########
@@ -18,23 +18,83 @@
*/
package org.apache.pulsar.common.policies.data.impl;
-import lombok.AllArgsConstructor;
-import lombok.Data;
+import lombok.EqualsAndHashCode;
import lombok.NoArgsConstructor;
+import lombok.ToString;
import org.apache.pulsar.common.policies.data.BacklogQuota;
-@Data
-@AllArgsConstructor
+@ToString
+@EqualsAndHashCode
@NoArgsConstructor
public class BacklogQuotaImpl implements BacklogQuota {
public static final long BYTES_IN_GIGABYTE = 1024 * 1024 * 1024;
- // backlog quota by size in byte
- private long limitSize;
- // backlog quota by time in second
+ /**
+ * backlog quota by size in byte, remains for compatible.
+ */
+ @Deprecated
+ private Long limit;
+
+ /**
+ * backlog quota by size in byte.
+ */
+ private Long limitSize;
+
+ /**
+ * backlog quota by time in second.
+ */
private int limitTime;
private RetentionPolicy policy;
+ public BacklogQuotaImpl(long limitSize, int limitTime, RetentionPolicy policy) {
+ this.limitSize = limitSize;
+ this.limitTime = limitTime;
+ this.policy = policy;
+ }
+
+ @Deprecated
+ public long getLimit() {
+ if (limitSize == null) {
+ // the limitSize and limit can't be both null
+ return limit;
+ }
+ return limitSize;
+ }
+
+ @Deprecated
+ public void setLimit(long limit) {
+ this.limit = limit;
+ this.limitSize = limit;
+ }
+
+ public long getLimitSize() {
+ if (limitSize == null) {
+ // the limitSize and limit can't be both null
+ return limit;
+ }
+ return limitSize;
+ }
+
+ public void setLimitSize(long limitSize) {
+ this.limitSize = limitSize;
Review comment:
If users use the new API, they will still encounter the problem, because `limit` will be 0, and for the old pulsar server, it only can accept the `limit` param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-994677461
Are we breaking compatibility with 2.8 ?
2.7 is far away, and the current release is 2.8...
I wonder if it is better to restore compatibility with 2.7 and break against 2.8 or is it better to keep compatibility with 2.8 and add another point to the "breaking changes" ?
because IIUC we broke compatibility in 2.8 and now (we are at 2.9) it is too late
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-994184153
@codelipenghui PTAL again, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-994690395
I have now understood better the final outcome of this patch.
I will cherry pick to branch-2.9 and prepare (again) 2.9.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-993483326
@merlimat @lhotari @eolivelli @codelipenghui PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-994231998
@eolivelli @linlinnn PTAL, and should this cherry pick to 2.8 and 2.9 ? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on a change in pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#discussion_r769290405
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/BacklogQuota.java
##########
@@ -28,6 +28,15 @@
*/
public interface BacklogQuota {
+ /**
+ * Gets quota limit in size.
+ * Remains for compatible
+ *
+ * @return quota limit in bytes
+ */
+ @Deprecated
Review comment:
I'd suggest to specify more the context here, maybe linking the PR and adding the "since" parameter to the @Deprecated annotation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj removed a comment on pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
Shoothzj removed a comment on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-993483326
@merlimat @lhotari @eolivelli @codelipenghui PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13291: Fix incompatibility of BacklogQuota
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#discussion_r769318003
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/BacklogQuota.java
##########
@@ -28,6 +28,15 @@
*/
public interface BacklogQuota {
+ /**
+ * Gets quota limit in size.
+ * Remains for compatible
+ *
+ * @return quota limit in bytes
+ */
+ @Deprecated
Review comment:
Added link and @since anootation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #13291: Fix incompatibility of GetBacklogQuota
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13291:
URL: https://github.com/apache/pulsar/pull/13291#issuecomment-994156515
@Shoothzj I noticed the fix only fixes the compatibility issue when getting backlog quota, I think another compatibility issue is when using a new version client >= 2.8 but the broker which < 2.8, since the broker-side don't have `limitSize` and the client side will not pass the `limit` to the broker, so that the broker will use `0` as the value of `value`, it also should be a compatibility issue.
It's better to add a compatibility test to make sure we will not introduce breaking change for this part.
Thanks for the great work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org