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/05/11 03:15:36 UTC

[GitHub] [pulsar] zymap opened a new pull request #10535: Allow to overwrite the offload drive config by metadata

zymap opened a new pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535


   ---
   
   *Motivation*
   We may change the offloaded data location by changing the metadata
   of the offloaded topic. So it should be allow to overwrite the driver
   config by metadata, and it should be fallback to use the local config
   if there isn't config in the metadata.
   
   In the currently implementation, it will never get the configurations
   from the local, because there always has value when reading offload.
   https://github.com/apache/pulsar/blob/4d2d66d17da73426e2281251f3b05a63218b70fb/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1746
   


-- 
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 #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535#issuecomment-839634030


   > My case is If we want to migrate the offload data, we may change the offload context and we can configure the cluster to use the new offload configurations to read offloaded data.
   
   If migrate the offload data, the offload context should be changed to the new offload configuration? If so, why not use the new offload context.


-- 
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 #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535#issuecomment-837948155


   Currently, this method is mainly used to get the bucket name of the offload data. If we use the topic offload configuration instead of the offload context in the LedgerInfo, this might make the managed ledger couldn't read the offload data?


-- 
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] zymap commented on pull request #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535#issuecomment-839554403


   @gaoran10 Yes. According to the code, it will fallback to use the offload configuration if there are no configurations in the offload context. 
   My case is If we want to migrate the offload data, we may change the offload context and we can configure the cluster to use the new offload configurations to read offloaded data. 


-- 
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] zymap commented on a change in pull request #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535#discussion_r630809805



##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java
##########
@@ -485,8 +485,9 @@ private PositionImpl lastOffered() {
      * @param offloadDriverMetadata
      * @return
      */
-    private BlobStoreLocation getBlobStoreLocation(Map<String, String> offloadDriverMetadata) {
-        return (!offloadDriverMetadata.isEmpty()) ? new BlobStoreLocation(offloadDriverMetadata) :
+    BlobStoreLocation getBlobStoreLocation(Map<String, String> offloadDriverMetadata) {
+        return (offloadDriverMetadata.keySet().containsAll(getOffloadDriverMetadata().keySet())) ?

Review comment:
       `getOffloadDriverMetadata()` will get the default metadata and the `offloadDriverMetadata` may contains more than the default value.




-- 
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] eolivelli commented on a change in pull request #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535#discussion_r629876241



##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java
##########
@@ -485,8 +485,9 @@ private PositionImpl lastOffered() {
      * @param offloadDriverMetadata
      * @return
      */
-    private BlobStoreLocation getBlobStoreLocation(Map<String, String> offloadDriverMetadata) {
-        return (!offloadDriverMetadata.isEmpty()) ? new BlobStoreLocation(offloadDriverMetadata) :
+    BlobStoreLocation getBlobStoreLocation(Map<String, String> offloadDriverMetadata) {
+        return (offloadDriverMetadata.keySet().containsAll(getOffloadDriverMetadata().keySet())) ?

Review comment:
       I cannot understand this comparison.
   shouldn't it be `offloadDriverMetadata.equals(getOffloadDriverMetadata())` ?
   otherwise we are only checking that there is no "new" entry 




-- 
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] zymap closed pull request #10535: Allow to overwrite the offload drive config by metadata

Posted by GitBox <gi...@apache.org>.
zymap closed pull request #10535:
URL: https://github.com/apache/pulsar/pull/10535


   


-- 
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