[GitHub] [carbondata] VenuReddy2103 opened a new pull request #3850: [CARBONDATA-3907]Reuse firePreLoadEvents and firePostLoadEvents methods from CommonLoadUtils to trigger LoadTablePreExecutionEvent and LoadTablePostExecutionEvent respectively in alter table add segment flow

[GitBox]

VenuReddy2103 opened a new pull request #3850:
URL: https://github.com/apache/carbondata/pull/3850


    ### Why is this PR needed?
   Currently we have 2 different ways of firing LoadTablePreExecutionEvent and LoadTablePostExecutionEvent. We can reuse firePreLoadEvents and firePostLoadEvents methods from CommonLoadUtils to trigger LoadTablePreExecutionEvent and LoadTablePostExecutionEvent respectively in alter table add segment flow as well. So that we can have single flow to fire these events
   
    ### What changes were proposed in this PR?
   Reuse firePreLoadEvents and firePostLoadEvents methods from CommonLoadUtils to trigger LoadTablePreExecutionEvent and LoadTablePostExecutionEvent respectively in alter table add segment flow.
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   


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

[GitBox]

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






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

[GitBox]

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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -228,24 +228,17 @@ case class CarbonAddLoadCommand(
     model.setTableName(carbonTable.getTableName)
     val operationContext = new OperationContext
     operationContext.setProperty("isLoadOrCompaction", false)
-    val loadTablePreExecutionEvent: LoadTablePreExecutionEvent =
-      new LoadTablePreExecutionEvent(
-        carbonTable.getCarbonTableIdentifier,
-        model)
-    operationContext.setProperty("isOverwrite", false)
-    OperationListenerBus.getInstance.fireEvent(loadTablePreExecutionEvent, operationContext)
-    // Add pre event listener for index indexSchema
-    val tableIndexes = IndexStoreManager.getInstance().getAllCGAndFGIndexes(carbonTable)
-    val indexOperationContext = new OperationContext()
-    if (tableIndexes.size() > 0) {
-      val indexNames: mutable.Buffer[String] =
-        tableIndexes.asScala.map(index => index.getIndexSchema.getIndexName)
-      val buildIndexPreExecutionEvent: BuildIndexPreExecutionEvent =
-        BuildIndexPreExecutionEvent(
-          sparkSession, carbonTable.getAbsoluteTableIdentifier, indexNames)
-      OperationListenerBus.getInstance().fireEvent(buildIndexPreExecutionEvent,
-        indexOperationContext)
-    }
+    val (tableIndexes, indexOperationContext) = CommonLoadUtils.firePreLoadEvents(

Review comment:
       @VenuReddy2103 can you confirm if at all please the same function is being used, if not change there also.




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

[GitBox]

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


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


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

[GitBox]

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


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


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

[GitBox]

VenuReddy2103 commented on a change in pull request #3850:
URL: https://github.com/apache/carbondata/pull/3850#discussion_r456466122



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonAddLoadCommand.scala
##########
@@ -228,24 +228,17 @@ case class CarbonAddLoadCommand(
     model.setTableName(carbonTable.getTableName)
     val operationContext = new OperationContext
     operationContext.setProperty("isLoadOrCompaction", false)
-    val loadTablePreExecutionEvent: LoadTablePreExecutionEvent =
-      new LoadTablePreExecutionEvent(
-        carbonTable.getCarbonTableIdentifier,
-        model)
-    operationContext.setProperty("isOverwrite", false)
-    OperationListenerBus.getInstance.fireEvent(loadTablePreExecutionEvent, operationContext)
-    // Add pre event listener for index indexSchema
-    val tableIndexes = IndexStoreManager.getInstance().getAllCGAndFGIndexes(carbonTable)
-    val indexOperationContext = new OperationContext()
-    if (tableIndexes.size() > 0) {
-      val indexNames: mutable.Buffer[String] =
-        tableIndexes.asScala.map(index => index.getIndexSchema.getIndexName)
-      val buildIndexPreExecutionEvent: BuildIndexPreExecutionEvent =
-        BuildIndexPreExecutionEvent(
-          sparkSession, carbonTable.getAbsoluteTableIdentifier, indexNames)
-      OperationListenerBus.getInstance().fireEvent(buildIndexPreExecutionEvent,
-        indexOperationContext)
-    }
+    val (tableIndexes, indexOperationContext) = CommonLoadUtils.firePreLoadEvents(

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]

[GitBox]

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



##########
File path: processing/src/main/java/org/apache/carbondata/processing/loading/events/LoadEvents.java
##########
@@ -51,12 +51,6 @@ public LoadTablePreExecutionEvent(CarbonTableIdentifier carbonTableIdentifier,
       this.isOverWriteTable = isOverWriteTable;
     }

Review comment:
       i think `LoadTablePostExecutionEvent` also not used now, please check and remove




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

[GitBox]

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


   @VenuReddy2103 please make PR heading short like below
   `Refactor to use CommonLoadUtils API's  firePreLoadEvents and firePostLoadEvents to trigger Load pre and post events`


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

[GitBox]

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


   retest this please


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

[GitBox]

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


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


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

[GitBox]

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


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


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

[GitBox]

VenuReddy2103 commented on a change in pull request #3850:
URL: https://github.com/apache/carbondata/pull/3850#discussion_r458109264



##########
File path: processing/src/main/java/org/apache/carbondata/processing/loading/events/LoadEvents.java
##########
@@ -51,12 +51,6 @@ public LoadTablePreExecutionEvent(CarbonTableIdentifier carbonTableIdentifier,
       this.isOverWriteTable = isOverWriteTable;
     }

Review comment:
       Have checked and found a listener(MVLoadPostEventListener) for it.




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

[GitBox]

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


   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]

[GitBox]

asfgit closed pull request #3850:
URL: https://github.com/apache/carbondata/pull/3850


   


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