[GitHub] [carbondata] akashrn5 opened a new pull request #3743: [WIP]Refactor system folder location and removed unwanted property

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 opened a new pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox

akashrn5 opened a new pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743


    ### Why is this PR needed?
   
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623593532


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1216/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623594555


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2934/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

Indhumathi27 commented on a change in pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#discussion_r419892328



##########
File path: core/src/main/java/org/apache/carbondata/core/view/MVProvider.java
##########
@@ -223,7 +224,7 @@ public void updateStatus(MVManager viewManager, List<MVSchema> schemaList,
 
   private void updateStatus(MVManager viewManager, String databaseName, List<MVSchema> schemaList,
       MVStatus status) throws IOException {
-    ICarbonLock carbonTableStatusLock = getStatusLock(databaseName);
+    ICarbonLock carbonTableStatusLock = getStatusLock(viewManager, databaseName);

Review comment:
       Can get databaseLocation and send only `databaseLocation` as parameter

##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1585,33 +1586,11 @@ public int getInvisibleSegmentPreserveCount() {
   }
 
   /**
-   * Get the configured system folder location.
-   * @return
-   */
-  public String getSystemFolderLocation() {
-    return getSystemFolderLocation(null);
-  }
-
-  /**
-   * Get the configured system folder location.
-   * @return
+   * Get the system folder location based on database location.
    */
-  public String getSystemFolderLocation(String databaseName) {
-    String systemLocation = CarbonProperties.getInstance()
-        .getProperty(CarbonCommonConstants.CARBON_SYSTEM_FOLDER_LOCATION);
-    if (systemLocation == null) {
-      systemLocation = getStorePath();
-    }
-    if (systemLocation != null) {
-      systemLocation = CarbonUtil.checkAndAppendFileSystemURIScheme(systemLocation);
-      systemLocation = FileFactory.getUpdatedFilePath(systemLocation);
-    }
-    if (databaseName == null) {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    } else {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR +
-          databaseName + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    }
+  public String getSystemFolderLocationPerDatabase(String databaseLocaltion) {

Review comment:
       Can move this method to MV module as it is specific to only mv.

##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1585,33 +1586,11 @@ public int getInvisibleSegmentPreserveCount() {
   }
 
   /**
-   * Get the configured system folder location.
-   * @return
-   */
-  public String getSystemFolderLocation() {
-    return getSystemFolderLocation(null);
-  }
-
-  /**
-   * Get the configured system folder location.
-   * @return
+   * Get the system folder location based on database location.
    */
-  public String getSystemFolderLocation(String databaseName) {
-    String systemLocation = CarbonProperties.getInstance()
-        .getProperty(CarbonCommonConstants.CARBON_SYSTEM_FOLDER_LOCATION);
-    if (systemLocation == null) {
-      systemLocation = getStorePath();
-    }
-    if (systemLocation != null) {
-      systemLocation = CarbonUtil.checkAndAppendFileSystemURIScheme(systemLocation);
-      systemLocation = FileFactory.getUpdatedFilePath(systemLocation);
-    }
-    if (databaseName == null) {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    } else {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR +
-          databaseName + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    }
+  public String getSystemFolderLocationPerDatabase(String databaseLocaltion) {

Review comment:
       ```suggestion
     public String getSystemFolderLocationPerDatabase(String databaseLocation) {
   ```




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

Indhumathi27 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623883763


   Please remove `carbon.system.folder.location` property from `integration/spark/src/main/scala/org/apache/spark/sql/test/util/QueryTest.scala`


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

akashrn5 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623887090


   > Please remove `carbon.system.folder.location` property from `integration/spark/src/main/scala/org/apache/spark/sql/test/util/QueryTest.scala`
   
   removed


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #3743: [WIP]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

akashrn5 commented on a change in pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#discussion_r419901153



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1585,33 +1586,11 @@ public int getInvisibleSegmentPreserveCount() {
   }
 
   /**
-   * Get the configured system folder location.
-   * @return
-   */
-  public String getSystemFolderLocation() {
-    return getSystemFolderLocation(null);
-  }
-
-  /**
-   * Get the configured system folder location.
-   * @return
+   * Get the system folder location based on database location.
    */
-  public String getSystemFolderLocation(String databaseName) {
-    String systemLocation = CarbonProperties.getInstance()
-        .getProperty(CarbonCommonConstants.CARBON_SYSTEM_FOLDER_LOCATION);
-    if (systemLocation == null) {
-      systemLocation = getStorePath();
-    }
-    if (systemLocation != null) {
-      systemLocation = CarbonUtil.checkAndAppendFileSystemURIScheme(systemLocation);
-      systemLocation = FileFactory.getUpdatedFilePath(systemLocation);
-    }
-    if (databaseName == null) {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    } else {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR +
-          databaseName + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    }
+  public String getSystemFolderLocationPerDatabase(String databaseLocaltion) {

Review comment:
       it is being referred by MVProvider and its in core, let it be in core module, cannot move to mv module as reference is before that. So it is ok to keep here

##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
##########
@@ -1585,33 +1586,11 @@ public int getInvisibleSegmentPreserveCount() {
   }
 
   /**
-   * Get the configured system folder location.
-   * @return
-   */
-  public String getSystemFolderLocation() {
-    return getSystemFolderLocation(null);
-  }
-
-  /**
-   * Get the configured system folder location.
-   * @return
+   * Get the system folder location based on database location.
    */
-  public String getSystemFolderLocation(String databaseName) {
-    String systemLocation = CarbonProperties.getInstance()
-        .getProperty(CarbonCommonConstants.CARBON_SYSTEM_FOLDER_LOCATION);
-    if (systemLocation == null) {
-      systemLocation = getStorePath();
-    }
-    if (systemLocation != null) {
-      systemLocation = CarbonUtil.checkAndAppendFileSystemURIScheme(systemLocation);
-      systemLocation = FileFactory.getUpdatedFilePath(systemLocation);
-    }
-    if (databaseName == null) {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    } else {
-      return systemLocation + CarbonCommonConstants.FILE_SEPARATOR +
-          databaseName + CarbonCommonConstants.FILE_SEPARATOR + "_system";
-    }
+  public String getSystemFolderLocationPerDatabase(String databaseLocaltion) {

Review comment:
       done

##########
File path: core/src/main/java/org/apache/carbondata/core/view/MVProvider.java
##########
@@ -223,7 +224,7 @@ public void updateStatus(MVManager viewManager, List<MVSchema> schemaList,
 
   private void updateStatus(MVManager viewManager, String databaseName, List<MVSchema> schemaList,
       MVStatus status) throws IOException {
-    ICarbonLock carbonTableStatusLock = getStatusLock(databaseName);
+    ICarbonLock carbonTableStatusLock = getStatusLock(viewManager, databaseName);

Review comment:
       done




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623940342


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1221/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-623945860


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2939/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

Indhumathi27 commented on a change in pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#discussion_r420024570



##########
File path: sdk/sdk/src/test/java/org/apache/carbondata/sdk/file/AvroCarbonWriterTest.java
##########
@@ -47,26 +47,6 @@
 public class AvroCarbonWriterTest {
   private String path = "./AvroCarbonWriterSuiteWriteFiles";
 
-  @Before

Review comment:
       Remove unused imports from all test case files from this PR




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

akashrn5 commented on a change in pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#discussion_r420607110



##########
File path: sdk/sdk/src/test/java/org/apache/carbondata/sdk/file/AvroCarbonWriterTest.java
##########
@@ -47,26 +47,6 @@
 public class AvroCarbonWriterTest {
   private String path = "./AvroCarbonWriterSuiteWriteFiles";
 
-  @Before

Review comment:
       removed, please check




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-624560326


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2947/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-624561080


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1229/
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

Indhumathi27 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-624569455


   LGTM


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] kunal642 commented on pull request #3743: [CARBONDATA-3792]Refactor system folder location and removed unwanted property

GitBox
In reply to this post by GitBox

kunal642 commented on pull request #3743:
URL: https://github.com/apache/carbondata/pull/3743#issuecomment-625249259


   LGTM


----------------------------------------------------------------
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:
[hidden email]