gortiz commented on code in PR #12704:
URL: https://github.com/apache/pinot/pull/12704#discussion_r1566905246


##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -36,108 +39,129 @@
  */
 public class MetadataBlock extends BaseDataBlock {
 
-  private static final ObjectMapper JSON = new ObjectMapper();
-
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(MetadataBlock.class);
   @VisibleForTesting
-  static final int VERSION = 1;
+  static final int VERSION = 2;

Review Comment:
   I think we are not going to add support for 2nd version in the code we have 
in master, but the code in this PR actually reads 1st and 2nd version. We don't 
need to do the former because we are lucky and the binary format should be 
compatible (though we may need to verify that adding a test in master).
   
   The implementation in both branches mostly ignore metrics in the other, but 
even they ignore the content, they are able to read them. We are just ignoring 
the content because we think it is not worth to actually convert from one 
format to the other given the impact will be to do not have all stats during 
the upgrade, but we could do that if needed.
   
   I think it is fine to increase the version.



-- 
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-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Reply via email to