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 2020/11/26 06:43:32 UTC
[GitHub] [pulsar] Renkai opened a new pull request #8717: Configurable data source for offloaded messages
Renkai opened a new pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717
Master issue: https://github.com/apache/pulsar/issues/8591
This PR include API change in command tools and related implementation with tests, related docs still in progress.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-738462414
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-737232988
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-735918733
Great work, maybe we could add a configuration like `tieredStorageReadPriority` at the config file `broker.conf` or `standalone.conf` for broker level configuration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r552390207
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1338,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
@Renkai Can we avoid using `System.out.println`?
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java
##########
@@ -42,9 +46,58 @@
@Data
public class OffloadPolicies implements Serializable {
+ @InterfaceAudience.Public
+ @InterfaceStability.Stable
+ public enum OffloadedReadPriority {
+ /**
+ * For offloaded messages, readers will try to read from bookkeeper at first,
+ * if messages not exist at bookkeeper then read from offloaded storage.
+ */
+ BOOKKEEPER_FIRST("bookkeeper-first"),
+ /**
+ * For offloaded messages, readers will try to read from offloaded storage first,
+ * even they are still exist in bookkeeper.
+ */
+ OFFLOADED_FIRST("offloaded-first");
Review comment:
I would suggest renaming `OFFLOADED_FIRST` to `TIERED_STORAGE_FIRST`.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1788,10 +1796,25 @@ void run() throws PulsarAdminException {
offloadAfterThresholdInBytes = offloadAfterThreshold;
}
}
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
@Renkai Can we avoid using `System.out.println`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-741315683
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r538029005
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1335,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ var offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
Review comment:
Let's avoid using `var` now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r537970086
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1536,7 +1537,18 @@ void asyncReadEntries(OpReadEntry opReadEntry) {
LedgerInfo info = ledgers.get(ledgerId);
CompletableFuture<ReadHandle> openFuture = new CompletableFuture<>();
- if (info != null && info.hasOffloadContext() && info.getOffloadContext().getComplete()) {
+
+ if (config.getLedgerOffloader() != null
+ && config.getLedgerOffloader().getOffloadPolicies() != null
+ && config.getLedgerOffloader().getOffloadPolicies()
Review comment:
> The `OffloadPolicies` you get from `config` may not be updated as namespace or topic policy update.
Good question, I will try to figure it out
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-750905946
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-739173143
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-738680948
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-739249966
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r552416951
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1788,10 +1796,25 @@ void run() throws PulsarAdminException {
offloadAfterThresholdInBytes = offloadAfterThreshold;
}
}
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
But this is a set command, no? Why people need this information when setting the offloaded priority?
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1338,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
This is a set command, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-737075571
> Great work, maybe we could add a configuration like `tieredStorageReadPriority` at the config file `broker.conf` or `standalone.conf` for broker level configuration.
@gaoran10
Now added to `broker.conf`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-754602714
> @hangc0276 Can you review this PR again?
@sijie done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-756547246
Great contributions!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-754115605
@hangc0276 Can you review this PR again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r552437894
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1338,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
@sijie I check the code again, I think it's ok to remove `println` so we can use mute as success information, if the value which user set is an invalid value, the tool will throw a exception. The `println` was removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-742221240
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-739127434
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r538172478
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1536,7 +1537,18 @@ void asyncReadEntries(OpReadEntry opReadEntry) {
LedgerInfo info = ledgers.get(ledgerId);
CompletableFuture<ReadHandle> openFuture = new CompletableFuture<>();
- if (info != null && info.hasOffloadContext() && info.getOffloadContext().getComplete()) {
+
+ if (config.getLedgerOffloader() != null
+ && config.getLedgerOffloader().getOffloadPolicies() != null
+ && config.getLedgerOffloader().getOffloadPolicies()
Review comment:
@hangc0276
I think
`org.apache.pulsar.broker.admin.v2.PersistentTopics#setOffloadPolicies`
`org.apache.pulsar.broker.admin.impl.PersistentTopicsBase#internalSetOffloadPolicies`
`org.apache.pulsar.broker.admin.impl.PersistentTopicsBase#internalUpdateOffloadPolicies`
`org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl#setConfig`
and
`org.apache.pulsar.zookeeper.ZooKeeperCache#process(org.apache.zookeeper.WatchedEvent, org.apache.pulsar.zookeeper.ZooKeeperCache.CacheUpdater<T>)`
`org.apache.pulsar.zookeeper.ZooKeeperChildrenCache#reloadCache`
`org.apache.pulsar.broker.service.BrokerService#onUpdate`
`org.apache.pulsar.broker.service.persistent.PersistentTopic#onPoliciesUpdate`
`org.apache.pulsar.broker.service.persistent.PersistentTopic#checkPersistencePolicies`
`org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl#setConfig`
are assurable chains to keep config up to date
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-737898611
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r552409547
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1338,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
@sijie This code is in a command-line tool, I think stdout is a good choice for users to get information
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1788,10 +1796,25 @@ void run() throws PulsarAdminException {
offloadAfterThresholdInBytes = offloadAfterThreshold;
}
}
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
ditto
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-738582407
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r549078828
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixTest.java
##########
@@ -23,22 +23,19 @@
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
-
import com.google.common.collect.ImmutableSet;
-
Review comment:
ditto
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r549080113
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1262,6 +1263,12 @@ public void openLedgerFailed(ManagedLedgerException exception, Object ctx) {
topicLevelOffloadPolicies,
OffloadPolicies.oldPoliciesCompatible(nsLevelOffloadPolicies, policies.orElse(null)),
getPulsar().getConfig().getProperties());
+
+ if (offloadPolicies != null && serviceConfig.getManagedLedgerDataReadPriority() != null) {
+ offloadPolicies.setManagedLedgerOffloadedReadPriority(
Review comment:
I removed these lines since the initialize progress should already be finished by ` OffloadPolicies.mergeConfiguration`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-785681204
@Renkai many thanks for your docs! While `site2/docs/cookbooks-tiered-storage.md` is deprecated. We will fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r537616077
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1335,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ var offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
Review comment:
not suggest to use `var`
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1536,7 +1537,18 @@ void asyncReadEntries(OpReadEntry opReadEntry) {
LedgerInfo info = ledgers.get(ledgerId);
CompletableFuture<ReadHandle> openFuture = new CompletableFuture<>();
- if (info != null && info.hasOffloadContext() && info.getOffloadContext().getComplete()) {
+
+ if (config.getLedgerOffloader() != null
+ && config.getLedgerOffloader().getOffloadPolicies() != null
+ && config.getLedgerOffloader().getOffloadPolicies()
Review comment:
The `OffloadPolicies` you get from `config` may not be updated as namespace or topic policy update.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-790320099
@Renkai I've submitted https://github.com/apache/pulsar/pull/9795 to add docs, could you please help review? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-737673729
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-737953582
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r548548467
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixReadTest.java
##########
@@ -39,7 +40,7 @@
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
-
+import lombok.val;
Review comment:
please remove this import
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1262,6 +1263,12 @@ public void openLedgerFailed(ManagedLedgerException exception, Object ctx) {
topicLevelOffloadPolicies,
OffloadPolicies.oldPoliciesCompatible(nsLevelOffloadPolicies, policies.orElse(null)),
getPulsar().getConfig().getProperties());
+
+ if (offloadPolicies != null && serviceConfig.getManagedLedgerDataReadPriority() != null) {
+ offloadPolicies.setManagedLedgerOffloadedReadPriority(
Review comment:
Maybe we should check whether offloadPolicies already have `ManagedLedgerOffloadedReadPriority` value.
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixReadTest.java
##########
@@ -69,53 +71,140 @@ public void testOffloadRead() throws Exception {
config.setRetentionTime(10, TimeUnit.MINUTES);
config.setRetentionSizeInMB(10);
config.setLedgerOffloader(offloader);
- ManagedLedgerImpl ledger = (ManagedLedgerImpl)factory.open("my_test_ledger", config);
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open("my_test_ledger", config);
for (int i = 0; i < 25; i++) {
String content = "entry-" + i;
ledger.addEntry(content.getBytes());
}
- Assert.assertEquals(ledger.getLedgersInfoAsList().size(), 3);
+ assertEquals(ledger.getLedgersInfoAsList().size(), 3);
ledger.offloadPrefix(ledger.getLastConfirmedEntry());
- Assert.assertEquals(ledger.getLedgersInfoAsList().size(), 3);
- Assert.assertEquals(ledger.getLedgersInfoAsList().stream()
- .filter(e -> e.getOffloadContext().getComplete()).count(), 2);
+ assertEquals(ledger.getLedgersInfoAsList().size(), 3);
+ assertEquals(ledger.getLedgersInfoAsList().stream()
+ .filter(e -> e.getOffloadContext().getComplete()).count(), 2);
Assert.assertTrue(ledger.getLedgersInfoAsList().get(0).getOffloadContext().getComplete());
Assert.assertTrue(ledger.getLedgersInfoAsList().get(1).getOffloadContext().getComplete());
Review comment:
`Assert.assertTrue` may sync with `assertEquals `
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixTest.java
##########
@@ -23,22 +23,19 @@
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
-
Review comment:
the blank line may not delete.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1434,15 +1433,19 @@
+ "Of course, this may degrade consumption throughput. Default is 10ms.")
private int managedLedgerNewEntriesCheckDelayInMillis = 10;
+ @FieldContext(category = CATEGORY_STORAGE_ML,
+ doc = "Read priority when ledgers exists in both bookkeeper and the second layer storage.")
+ private String managedLedgerDataReadPriority = OffloadPolicies.OffloadedReadPriority.OFFLOADED_FIRST.getValue();
+
/*** --- Load balancer --- ****/
@FieldContext(
- category = CATEGORY_LOAD_BALANCER,
- doc = "Enable load balancer"
+ category = CATEGORY_LOAD_BALANCER,
+ doc = "Enable load balancer"
)
private boolean loadBalancerEnabled = true;
@Deprecated
@FieldContext(
- category = CATEGORY_LOAD_BALANCER,
+ category = CATEGORY_LOAD_BALANCER,
Review comment:
no blank
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixTest.java
##########
@@ -23,22 +23,19 @@
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
-
import com.google.common.collect.ImmutableSet;
-
import java.lang.reflect.Field;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
-import java.util.concurrent.CompletionException;
import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.function.BooleanSupplier;
import java.util.stream.Collectors;
-
Review comment:
same with upper.
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixTest.java
##########
@@ -23,22 +23,19 @@
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
-
import com.google.common.collect.ImmutableSet;
-
Review comment:
the blank line may not delete.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -21,9 +21,7 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
-
Review comment:
may not delete the new line.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -25,13 +25,12 @@
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
-
import java.io.IOException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.concurrent.TimeUnit;
-
+import java.util.stream.Collectors;
import org.apache.commons.lang3.StringUtils;
Review comment:
add new blank
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#issuecomment-734170680
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r537973387
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1336,11 +1335,35 @@ void run() throws PulsarAdminException {
, description = "ManagedLedger offload deletion lag in bytes")
private Long offloadDeletionLagInMillis;
+ @Parameter(
+ names = {"--offloadedReadPriority", "-orp"},
+ description = "read priority for offloaded messages",
+ required = false
+ )
+ private String offloadReadPriorityStr;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
+
+ var offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
Review comment:
@hangc0276
The official support of type inference in Java began at Java 10, I think it's a good feature to keep code base concise, before Java 10, we can use the Lombok version first, just like we use JodaTime before Java8.
https://openjdk.java.net/jeps/286
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r552438406
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1788,10 +1796,25 @@ void run() throws PulsarAdminException {
offloadAfterThresholdInBytes = offloadAfterThreshold;
}
}
+ OffloadedReadPriority offloadedReadPriority = OffloadPolicies.DEFAULT_OFFLOADED_READ_PRIORITY;
+
+ if (this.offloadReadPriorityStr != null) {
+ try {
+ offloadedReadPriority = OffloadedReadPriority.fromString(this.offloadReadPriorityStr);
+ System.out.println("offloadedReadPriority = " + offloadedReadPriority);
Review comment:
ditto
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Renkai commented on a change in pull request #8717: Configurable data source for offloaded messages
Posted by GitBox <gi...@apache.org>.
Renkai commented on a change in pull request #8717:
URL: https://github.com/apache/pulsar/pull/8717#discussion_r549078801
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/OffloadPrefixTest.java
##########
@@ -23,22 +23,19 @@
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
-
Review comment:
The checkstyle https://github.com/apache/pulsar/blob/cc64889abe94d47f048e2f8e8fb10d6c37e695ec/buildtools/src/main/resources/pulsar/checkstyle.xml need us to leave no blank line between imports, though it's not enabled in all modules yet, but it will. So I think it's better to remove blank lines here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org