[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5085: Root disk size should be listed in GB at listServiceOffering

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

sureshanaparti commented on a change in pull request #5085:
URL: https://github.com/apache/cloudstack/pull/5085#discussion_r647998852



##########
File path: server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java
##########
@@ -45,6 +45,13 @@
 
     private SearchBuilder<ServiceOfferingJoinVO> sofIdSearch;
 
+    /**
+     * Constant used to convert GB into Bytes (or the other way around).
+     * GB   *  MB  *  KB  = Bytes //
+     * 1024 * 1024 * 1024 = 1073741824
+     */
+    private static final long GB_TO_BYTES = 1073741824;

Review comment:
       multiple constants are defined, in various classes, for the value "1024 * 1024 * 1024", and it is hardcoded in some cases. may be (later), a constant can be defined in common package( eg. cloud utils), and use it across. (Such common util functions has to be documented, so that the developers can use these)
   
   Also, some get disk size calls return size in Bytes, and others return in GB. I think, it is better to explicitly mention (in variables / methods) it indicating Bytes or GB for non-default unit (i.e. If Bytes is the default unit, then methods which return in 'GB' should have it mentioned). If there is no such default unit, then variables / methods should indicate the size units explicitly. This can be a huge refactoring work (so can be skipped for now) and at least, have to make sure new code indicates proper size unit.




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