[HOTFIX] Listeners not getting registered to the bus in CarbonSessionState Implementations #1821

kunal642 · 2018-01-17T10:06:30Z

Problem: Listeners are not getting registered if you create a new implementation of CarbonSessionState and add it to spark using configuration. In this case CarbonSession would not be created and thus listeners are not registered.

Solution: Register listeners in CarbonSessionState instead of CarbonSession.

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed?
Any backward compatibility impacted?
Document update required?
Testing done
Please provide details on
- Whether new unit test cases have been added or why no new tests are required?
- How it is tested? Please attach test report.
- Is it a performance related change? Please attach the performance test report.
- Any additional information to help reviewers in testing this change.
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

KanakaKumar · 2018-01-17T12:25:19Z

core/src/main/java/org/apache/carbondata/events/OperationListenerBus.java

    if (null == operationEventListeners) {
-      operationEventListeners = new CopyOnWriteArrayList<>();
+      operationEventListeners = new CopyOnWriteArraySet<>();


Insertion order of listeners is important. Please use LinkedHashSet

Also, we need ensure only one object for listener type is added. One way is to use wrapper object with equals, hashcode on Qualified Class name.

Changed the code to use addIfAbsent method instead of add. This will take care of dublicate entries.

changed the OperationEventListener interface to abstract class with equals and hashcode which will compare the class name.

CarbonDataQA · 2018-01-17T12:36:38Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2890/

CarbonDataQA · 2018-01-17T12:36:53Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1657/

CarbonDataQA · 2018-01-17T13:32:25Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2893/

CarbonDataQA · 2018-01-17T14:41:09Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1668/

CarbonDataQA · 2018-01-17T14:43:24Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2899/

CarbonDataQA · 2018-01-17T17:00:07Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1673/

CarbonDataQA · 2018-01-17T17:00:13Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2905/

ravipesala · 2018-01-17T17:49:06Z

@kunal642 Please fix checkstyle issues

CarbonDataQA · 2018-01-17T21:43:13Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1686/

CarbonDataQA · 2018-01-17T23:27:15Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2919/

jackylk · 2018-01-18T07:21:52Z

integration/spark2/src/main/spark2.1/CarbonSessionState.scala

@@ -75,6 +75,8 @@ class CarbonSessionCatalog(
    env
  }

+  CarbonEnv.initListeners()


There is a CarbonEnv.init function, can you put this initialization inside that

jackylk · 2018-01-18T07:22:30Z

core/src/main/java/org/apache/carbondata/events/OperationEventListener.java

+    return getClass().getName();
+  }
+
+  @Override public int hashCode() {


move @OverRide to previous line

kunal642 · 2018-01-18T07:54:12Z

@jackylk handled all the review comments.

ravipesala · 2018-01-18T08:09:38Z

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2954/

jackylk · 2018-01-18T08:18:34Z

integration/spark2/src/main/spark2.2/CarbonSessionState.scala

@@ -216,6 +216,8 @@ class CarbonSessionStateBuilder(sparkSession: SparkSession,
    new CarbonUDFTransformRule,
    new CarbonLateDecodeRule)

+  CarbonEnv.initListeners()


The same, this is moved to CarbonEnv.init

CarbonDataQA · 2018-01-18T10:47:56Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1710/

CarbonDataQA · 2018-01-18T11:20:17Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2944/

ravipesala · 2018-01-18T13:24:00Z

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2973/

CarbonDataQA · 2018-01-18T13:40:20Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1719/

ravipesala · 2018-01-18T17:16:11Z

retest this please

ravipesala · 2018-01-18T18:32:49Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2983/

CarbonDataQA · 2018-01-18T19:55:56Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1735/

CarbonDataQA · 2018-01-18T19:55:57Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2969/

CarbonDataQA · 2018-01-18T21:55:56Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1739/

ravipesala · 2018-01-19T06:15:15Z

@kunal642 It is better to remove lazy val for carbonenv in carbonsessionstate to make sure it registers while creating session.

ravipesala · 2018-01-19T09:40:03Z

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2994/

CarbonDataQA · 2018-01-19T09:58:46Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2981/

CarbonDataQA · 2018-01-19T09:58:49Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1751/

CarbonDataQA · 2018-01-19T11:02:47Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1755/

CarbonDataQA · 2018-01-19T11:55:56Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2985/

ravipesala · 2018-01-19T11:56:08Z

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2998/

kunal642 · 2018-01-19T11:57:22Z

retest sdv please

CarbonDataQA · 2018-01-19T12:02:47Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1758/

ravipesala · 2018-01-19T12:35:06Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2999/

CarbonDataQA · 2018-01-19T12:55:57Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2988/

ravipesala · 2018-01-19T13:16:08Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3000/

ravipesala · 2018-01-19T13:45:30Z

LGTM

CarbonDataQA · 2018-01-19T13:55:56Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2991/

CarbonDataQA · 2018-01-19T13:55:56Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/1761/

ravipesala · 2018-01-19T14:02:13Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3002/

ravipesala · 2018-01-19T14:44:24Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3003/

…State Implementations Problem: Listeners are not getting registered if you create a new implementation of CarbonSessionState and add it to spark using configuration. In this case CarbonSession would not be created and thus listeners are not registered. Solution: Register listeners in CarbonSessionState instead of CarbonSession. This closes apache#1821

rahulk2 force-pushed the preagg_refactor branch from b95da5b to 5b41ec5 Compare January 17, 2018 10:40

KanakaKumar reviewed Jan 17, 2018

View reviewed changes

rahulk2 force-pushed the preagg_refactor branch from 5b41ec5 to 74cbba0 Compare January 17, 2018 13:33

kunal642 changed the title ~~[WIP]refactored code to support external session state impl~~ [HOTFIX] Listeners not getting registered to the bus in CarbonSessionState Implementations Jan 17, 2018

rahulk2 force-pushed the preagg_refactor branch from 74cbba0 to 49bc938 Compare January 17, 2018 16:24

rahulk2 force-pushed the preagg_refactor branch from 49bc938 to 411c7a1 Compare January 17, 2018 18:03

jackylk reviewed Jan 18, 2018

View reviewed changes

rahulk2 force-pushed the preagg_refactor branch from 411c7a1 to 21bd909 Compare January 18, 2018 07:53

jackylk reviewed Jan 18, 2018

View reviewed changes

rahulk2 force-pushed the preagg_refactor branch from 21bd909 to 8a87d82 Compare January 18, 2018 09:49

rahulk2 force-pushed the preagg_refactor branch from 8a87d82 to cd78e0c Compare January 18, 2018 11:06

rahulk2 force-pushed the preagg_refactor branch from cd78e0c to 04d23d9 Compare January 18, 2018 14:47

rahulk2 force-pushed the preagg_refactor branch 2 times, most recently from b353edb to e9458b2 Compare January 19, 2018 08:23

rahulk2 force-pushed the preagg_refactor branch 2 times, most recently from 815b8e1 to 2f5a69a Compare January 19, 2018 10:36

rahulk2 force-pushed the preagg_refactor branch from 2f5a69a to 2d684c9 Compare January 19, 2018 11:40

refactored code to support external session state impl

db2240c

rahulk2 force-pushed the preagg_refactor branch from 2d684c9 to db2240c Compare January 19, 2018 12:08

asfgit closed this in 937868d Jan 19, 2018

[HOTFIX] Listeners not getting registered to the bus in CarbonSessionState Implementations #1821

[HOTFIX] Listeners not getting registered to the bus in CarbonSessionState Implementations #1821

Conversation

kunal642 commented Jan 17, 2018 • edited

KanakaKumar Jan 17, 2018 • edited

Choose a reason for hiding this comment

kunal642 Jan 17, 2018

Choose a reason for hiding this comment

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

ravipesala commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

CarbonDataQA commented Jan 17, 2018

jackylk Jan 18, 2018

Choose a reason for hiding this comment

jackylk Jan 18, 2018

Choose a reason for hiding this comment

kunal642 commented Jan 18, 2018

ravipesala commented Jan 18, 2018

jackylk Jan 18, 2018

Choose a reason for hiding this comment

CarbonDataQA commented Jan 18, 2018

CarbonDataQA commented Jan 18, 2018

ravipesala commented Jan 18, 2018

CarbonDataQA commented Jan 18, 2018

ravipesala commented Jan 18, 2018

ravipesala commented Jan 18, 2018

CarbonDataQA commented Jan 18, 2018

CarbonDataQA commented Jan 18, 2018

CarbonDataQA commented Jan 18, 2018

ravipesala commented Jan 19, 2018

ravipesala commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

ravipesala commented Jan 19, 2018

kunal642 commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

ravipesala commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

ravipesala commented Jan 19, 2018

ravipesala commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

CarbonDataQA commented Jan 19, 2018

ravipesala commented Jan 19, 2018

ravipesala commented Jan 19, 2018

kunal642 commented Jan 17, 2018 •

edited

KanakaKumar Jan 17, 2018 •

edited