[GitHub] [pulsar] poorbarcode commented on a diff in pull request #19913: [fix][broker]Make LedgerOffloaderFactory compatible with old version.

["poorbarcode (via GitHub)" <gi...@apache.org>]

poorbarcode commented on code in PR #19913:
URL: https://github.com/apache/pulsar/pull/19913#discussion_r1155096375


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/LedgerOffloaderFactory.java:
##########
@@ -42,20 +42,37 @@
     boolean isDriverSupported(String driverName);
 
     /**
-     * Create a ledger offloader with the provided configuration, user-metadata and scheduler.
+     * Create a ledger offloader with the provided configuration, user-metadata, scheduler and offloaderStats.
      *
      * @param offloadPolicies offload policies
      * @param userMetadata user metadata
      * @param scheduler scheduler
      * @return the offloader instance
      * @throws IOException when fail to create an offloader
      */
+    T create(OffloadPoliciesImpl offloadPolicies,
+             Map<String, String> userMetadata,
+             OrderedScheduler scheduler)
+            throws IOException;
+
+
+    /**
+     * Create a ledger offloader with the provided configuration, user-metadata, scheduler and offloaderStats.
+     *
+     * @param offloadPolicies offload policies
+     * @param userMetadata user metadata
+     * @param scheduler scheduler
+     * @param offloaderStats offloaderStats
+     * @return the offloader instance
+     * @throws IOException when fail to create an offloader
+     */
     T create(OffloadPoliciesImpl offloadPolicies,
              Map<String, String> userMetadata,
              OrderedScheduler scheduler,
              LedgerOffloaderStats offloaderStats)
         throws IOException;

Review Comment:
   There are four scenarios:
   - `old nar & old broker`: I think it will work. 
   - `old nar & new broker`: It will not work, and we do not guarantee forward compatibility. So it is okay.
   - `new nar & old broker`: The broker will call the method without `@param offloaderStats.` I think it will work.
     - we should make the method without `@param offloaderStats` non-default.
   - `new nar & new broker` I think it will work.
   
   All things are good.



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