[GitHub] [hudi] guykhazma commented on a change in pull request #2231: [HUDI-1373] Add Support for OpenJ9 JVM

[GitBox <gi...@apache.org>]

guykhazma commented on a change in pull request #2231:
URL: https://github.com/apache/hudi/pull/2231#discussion_r528798516



##########
File path: hudi-common/src/main/java/org/apache/hudi/common/util/ObjectSizeCalculator.java
##########
@@ -328,54 +328,53 @@ private static long getPrimitiveFieldSize(Class<?> type) {
   static MemoryLayoutSpecification getEffectiveMemoryLayoutSpecification() {
     final String vmName = System.getProperty("java.vm.name");
     if (vmName == null || !(vmName.startsWith("Java HotSpot(TM) ") || vmName.startsWith("OpenJDK")
-        || vmName.startsWith("TwitterJDK"))) {
-      throw new UnsupportedOperationException("ObjectSizeCalculator only supported on HotSpot VM");
+        || vmName.startsWith("TwitterJDK") || vmName.startsWith("Eclipse OpenJ9"))) {
+      throw new UnsupportedOperationException("ObjectSizeCalculator only supported on HotSpot or Eclipse OpenJ9 VMs");
     }
 
-    final String dataModel = System.getProperty("sun.arch.data.model");
-    if ("32".equals(dataModel)) {
-      // Running with 32-bit data model
-      return new MemoryLayoutSpecification() {
-        @Override
-        public int getArrayHeaderSize() {
-          return 12;
-        }
+    final String strVmVersion = System.getProperty("java.vm.version");
+    // Support for OpenJ9 JVM
+    if (strVmVersion.startsWith("openj9")) {
+      final String dataModel = System.getProperty("sun.arch.data.model");
+      if ("32".equals(dataModel)) {
+        // Running with 32-bit data model
+        return new MemoryLayoutSpecification() {

Review comment:
       agree, wasn't sure if it was a good idea to change this as part of this PR but I am happy to make the change.
   I was thinking of having `HotSpotMemoryLayoutSpecification` and `OpenJ9MemoryLayoutSpecification` classes, both will implement the `MemoryLayoutSpecification` interface and will get as a parameter the `dataModel` that will help it to determine whether it's 32 or 64 bit.
   
   is this what you had in mind?
   the downside of this is that each function will in the interface will have multiple if's inside it. for example:
   ```Java
   @Override
             public int getArrayHeaderSize() {
               if ("32".equals(dataModel)) {
                 return 16;
               } // assuming here 64 bit 
               else {
                 if (maxMemory < 30L * 1024 * 1024 * 1024) {
                   return 16;
                 } else {
                   return 14;
                 }
               }
             }
   ```
   
   another option is to have class per each combination (hotspot 32 bit, hotspot 64 bit compressed, hotspot 64 bit non compressed, openj9 32 bit, openj9 64 bit compressed, and openj9 64 bit non compressed)




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