[GitHub] [carbondata] akashrn5 opened a new pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue

[GitBox]

akashrn5 opened a new pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642
 
 
    ### 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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-592385093
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/516/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-592411490
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2216/
   

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r385988707
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -269,26 +275,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    * @param providerName
    * @return
    */
-  public synchronized DataMapCatalog getDataMapCatalog(
-      DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+  public synchronized DataMapCatalog getDataMapCatalog(DataMapProvider dataMapProvider,
 
 Review comment:
   move `DataMapProvider dataMapProvider,` to next line

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r385988813
 
 

 ##########
 File path: mv/core/src/main/scala/org/apache/carbondata/mv/rewrite/SummaryDatasetCatalog.scala
 ##########
 @@ -107,12 +110,19 @@ private[mv] class SummaryDatasetCatalog(sparkSession: SparkSession)
       // catalog, if the datamap is in database other than sparkSession.currentDataBase(), then it
       // fails to register, so set the database present in the dataMapSchema Object
       setCurrentDataBase(dataMapSchema.getRelationIdentifier.getDatabaseName)
-      val mvPlan = MVParser.getMVPlan(dataMapSchema.getCtasQuery, sparkSession)
-      // here setting back to current database of current session, because if the actual query
-      // contains db name in query like, select db1.column1 from table and current database is
-      // default and if we drop the db1, still the session has current db as db1.
-      // So setting back to current database.
-      setCurrentDataBase(currentDatabase)
+      val mvPlan = try {
+        MVParser.getMVPlan(dataMapSchema.getCtasQuery, sparkSession)
+      } catch {
+        case ex: Exception =>
+          LOGGER.error("Error executing the updated query during register datamap schema", ex)
 
 Review comment:
   ```suggestion
             LOGGER.error("Error executing the updated query during register MV schema", ex)
   ```

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r385988945
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapCatalog.java
 ##########
 @@ -48,4 +48,9 @@
    */
   void refresh();
 
+  /**
+   * This checks whether the datamapSchema is already registered
+   */
+  Boolean isSchemaAlreadyRegistered(String dataMapName);
 
 Review comment:
   ```suggestion
     Boolean isMVExists(String mvName);
   ```

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386210430
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapCatalog.java
 ##########
 @@ -48,4 +48,9 @@
    */
   void refresh();
 
+  /**
+   * This checks whether the datamapSchema is already registered
+   */
+  Boolean isSchemaAlreadyRegistered(String dataMapName);
 
 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]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386210452
 
 

 ##########
 File path: mv/core/src/main/scala/org/apache/carbondata/mv/rewrite/SummaryDatasetCatalog.scala
 ##########
 @@ -107,12 +110,19 @@ private[mv] class SummaryDatasetCatalog(sparkSession: SparkSession)
       // catalog, if the datamap is in database other than sparkSession.currentDataBase(), then it
       // fails to register, so set the database present in the dataMapSchema Object
       setCurrentDataBase(dataMapSchema.getRelationIdentifier.getDatabaseName)
-      val mvPlan = MVParser.getMVPlan(dataMapSchema.getCtasQuery, sparkSession)
-      // here setting back to current database of current session, because if the actual query
-      // contains db name in query like, select db1.column1 from table and current database is
-      // default and if we drop the db1, still the session has current db as db1.
-      // So setting back to current database.
-      setCurrentDataBase(currentDatabase)
+      val mvPlan = try {
+        MVParser.getMVPlan(dataMapSchema.getCtasQuery, sparkSession)
+      } catch {
+        case ex: Exception =>
+          LOGGER.error("Error executing the updated query during register datamap schema", ex)
 
 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]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386210476
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -269,26 +275,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    * @param providerName
    * @return
    */
-  public synchronized DataMapCatalog getDataMapCatalog(
-      DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+  public synchronized DataMapCatalog getDataMapCatalog(DataMapProvider dataMapProvider,
 
 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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-593240615
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/556/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-593263719
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2256/
   

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-593732694
 
 
   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]


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386765528
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -271,24 +277,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    */
   public synchronized DataMapCatalog getDataMapCatalog(
       DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+      String providerName,
+      boolean clearCatalogs) throws IOException {
 
 Review comment:
   This is not clear, maybe it is better to add a separate API to clear the catalog

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386765528
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -271,24 +277,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    */
   public synchronized DataMapCatalog getDataMapCatalog(
       DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+      String providerName,
+      boolean clearCatalogs) throws IOException {
 
 Review comment:
   This is not clear, maybe it is better to add a separate API to clear the catalog
   @ajantha-bhat please check once

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


With regards,
Apache Git Services

[GitBox]

jackylk removed a comment on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-593732694
 
 
   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]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386800566
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -271,24 +277,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    */
   public synchronized DataMapCatalog getDataMapCatalog(
       DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+      String providerName,
+      boolean clearCatalogs) throws IOException {
 
 Review comment:
   initialy we had a separate API for clear, i have removed in this PR because it was synchronized and instead of calling that API everytime if session changes, it better if we handle here where we intialize catalogs and its synchronized also, so it takes care of all.

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r386800566
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -271,24 +277,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    */
   public synchronized DataMapCatalog getDataMapCatalog(
       DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+      String providerName,
+      boolean clearCatalogs) throws IOException {
 
 Review comment:
   initially we had a separate API for clear, i have removed in this PR because it was not synchronized and instead of calling that API everytime if session changes(Intention is to reduce API calls), it better if we handle here where we intialize catalogs and its synchronized also, so it takes care of all.

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#discussion_r387089006
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -271,24 +277,23 @@ public synchronized void unRegisterDataMapCatalog(DataMapSchema dataMapSchema) {
    */
   public synchronized DataMapCatalog getDataMapCatalog(
       DataMapProvider dataMapProvider,
-      String providerName) throws IOException {
+      String providerName,
+      boolean clearCatalogs) throws IOException {
 
 Review comment:
   @jackylk : This is ok, clear is just assigning to null and multiple function calls in concurrent scenario always better to avoid.
   However, initializeDataMapCatalogs can be optimized in the future with more testing. we have some duplicate code there to handle the concurrent scenario

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on issue #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642#issuecomment-594356701
 
 
   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]


With regards,
Apache Git Services

[GitBox]

asfgit closed pull request #3642: [CARBONDATA-3725]fix concurrent creation of Materialized Views issue
URL: https://github.com/apache/carbondata/pull/3642
 
 
   

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


With regards,
Apache Git Services