Feature/repository datastax migration performance #146

bargenilesh · 2017-01-06T00:51:24Z

Batch assignment flow optimizations...

…mize the impact of increase in # of experiments.

coveralls · 2017-01-06T00:58:58Z

Coverage increased (+0.02%) to 73.62% when pulling 3c03059 on feature/repository-datastax-migration-performance into 169002b on feature/repository-datastax-migration.

felixgao · 2017-01-06T01:08:31Z

modules/assignment/src/main/java/com/intuit/wasabi/assignment/impl/AssignmentsImpl.java

-        Table<Experiment.ID, Experiment.Label, Experiment> allExperiments = repository.getExperimentList(applicationName);
+        long sTime,totalSTime;
+        totalSTime=System.currentTimeMillis();
+        sTime=System.currentTimeMillis();


move this line to above line like long sTime=System.currentTimeMillis(), totalSTime=System.currentTimeMillis();

Got rid of sTime and totalSTime as I have removed performance logger...

felixgao · 2017-01-06T01:10:13Z

modules/assignment/src/main/java/com/intuit/wasabi/assignment/impl/AssignmentsImpl.java


-        List<Map> allAssignments = new ArrayList<>();
+        //allowAssignments is NULL when called from AssignmentsResource.getBatchAssignments()
+        Set<Experiment.ID> experimentIds = allowAssignments==null?null:allowAssignments.keySet();


We return remove null from the code as much as possible, either chagne allowAssignments to Optional or use other syntax like isNull(allowAssignments) ? Set.EMPTY() : allowAssignments.keySet();

Changed allowAssignments to Optional and then related changes...

felixgao · 2017-01-06T01:10:56Z

modules/assignment/src/main/java/com/intuit/wasabi/assignment/impl/AssignmentsImpl.java


+        List<Map> allAssignments = new ArrayList<>();


LInkedList instead of ArrayList for dynamic sized List

Changed to LinkedList..

felixgao · 2017-01-06T01:12:52Z

modules/assignment/src/main/java/com/intuit/wasabi/assignment/impl/AssignmentsImpl.java

        // iterate over all experiments in the application in priority order
        for (PrioritizedExperiment experiment : appPriorities.getPrioritizedExperiments()) {
+            if(LOGGER.isDebugEnabled()) LOGGER.debug("Now processing: {}", experiment);


not really need isDebugEnabled since the debug message is not expensive operations at all

Added this as having isDebugEnabled avoids a string concatenation in the case if logging LEVEL is not debug...

felixgao · 2017-01-06T01:13:15Z

modules/assignment/src/main/java/com/intuit/wasabi/assignment/impl/AssignmentsImpl.java

@@ -458,12 +503,11 @@ public Assignment getAssignment(User.ID userID, Application.Name applicationName
                        .build();

                try {
-                    Assignment assignment = getAssignment(userID, applicationName, label,
+                      Assignment assignment = getAssignment(userID, applicationName, label,


remove allowAssingments != null from code

Although this was existing code, changed to use Optional implementation...

felixgao · 2017-01-06T01:28:50Z

...rc/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraAssignmentsRepository.java

+        if(logger.isDebugEnabled()) logger.debug("experimentMap=> {}", experimentMap);
+
+        int priorityValue=1;
+        for (com.intuit.wasabi.repository.cassandra.pojo.Application priority : UninterruptibleUtil.getUninterruptibly(applicationFuture).all()) {


there is already an getUninterruptibly function from guava, since there isn't much we can do with the exception it is better just log it and move on.

Please refer to the above comments...

felixgao · 2017-01-06T01:29:35Z

...rc/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraAssignmentsRepository.java

+        UninterruptibleUtil.getUninterruptibly(userAssignmentsFuture).all().stream().forEach((ExperimentUserByUserIdContextAppNameExperimentId uaPojo) -> {
+            Experiment.ID experimentID = Experiment.ID.valueOf(uaPojo.getExperimentId());
+            Experiment exp = experimentMap.get(experimentID);
+            if(exp!=null) {


felixgao · 2017-01-06T01:29:42Z

...rc/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraAssignmentsRepository.java

+
+        //experimentIds is NULL means experimentBatch.labels are present.
+        //Use experimentBatch.labels to populate experimentIds
+        if(experimentIds==null) {


Changed to Optional implementation as part of one of the previous comment..

felixgao · 2017-01-06T01:29:55Z

...rc/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraAssignmentsRepository.java

+            Set<Experiment.Label> expLabels = new HashSet<>();
+            for(Experiment.ID expId:experimentIds) {
+                Experiment exp = experimentMap.get(expId);
+                if(exp!=null) {


Already taken care from one of the previous comment...

felixgao · 2017-01-06T01:31:05Z

...stax/src/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraPagesRepository.java

+            Result<PageExperimentByAppNamePage> result = pageExperimentIndexAccessor.selectBy(applicationName.toString(), pageName.toString());
+            resultList = StreamSupport.stream(Spliterators.spliteratorUnknownSize(result.iterator(), Spliterator.ORDERED), false);
+        } catch (ReadTimeoutException | UnavailableException | NoHostAvailableException e) {
+            throw new RepositoryException("Could not retrieve the experiments for applicationName:\"" + applicationName + "\", page:\"" + pageName, e);


let's not do string concat here

Changed to use StringBuilder... Note: This implementation was done similar to the other methods in the same class and so copied and then modified and so other methods are still doing string concatenation...

mans4singh · 2017-01-09T15:56:38Z

ATTRIBUTIONS/wasabi-attribution.xml

@@ -611,4 +611,4 @@
         </downloadUrls>
      </dependency>
   </dependencies>
-</attributionReport>


We can remove this change.

This file changed automatically by build... What change do you suggest to do here?

I think if we just check out the file and the difference will be deleted. But it is just a minor issue.

ok... got it.

mans4singh · 2017-01-09T15:59:31Z

...y-datastax/src/main/java/com/intuit/wasabi/repository/cassandra/accessor/BucketAccessor.java

@@ -45,6 +46,9 @@
    @Query("select * from bucket where experiment_id = ?")


Should we remove the sync method ?

Need to keep this method as it is still being used in other places other than batch-assignment.

That was my suggestion - can we use asnyc everywhere ?

yes, we could and should bit in this feature I am trying to keep scope limited to Batch assignment only...

mans4singh · 2017-01-09T16:00:08Z

...atastax/src/main/java/com/intuit/wasabi/repository/cassandra/accessor/ExclusionAccessor.java

@@ -37,6 +38,9 @@
    @Query("select * from exclusion where base = ?")
    Result<Exclusion> getExclusions(UUID base);


Should we remove the sync method since we are using async one ?

Need to keep this method as it is still being used in other places other than batch-assignment.

mans4singh · 2017-01-09T16:01:04Z

.../java/com/intuit/wasabi/repository/cassandra/accessor/index/ExperimentUserIndexAccessor.java

@@ -40,6 +41,10 @@
    @Query("select * from experiment_user_index where user_id = ? and app_name = ? and context = ?")
    Result<ExperimentUserByUserIdContextAppNameExperimentId> selectBy(String userId, String appName, String context);


Same as above - remove sync method.

Need to keep this method as it is still being used in other places other than batch-assignment

mans4singh · 2017-01-09T16:01:23Z

.../java/com/intuit/wasabi/repository/cassandra/accessor/index/PageExperimentIndexAccessor.java

@@ -40,7 +40,7 @@
            "values(?,?,?,?)")
    Statement insertBy(String appName, String page, UUID experimentId, boolean assign);

-    /**
+    /**experi


Is this a typo ?

Corrected typo...

mans4singh · 2017-01-09T16:03:47Z

...rc/main/java/com/intuit/wasabi/repository/cassandra/impl/CassandraAssignmentsRepository.java

 import com.intuit.wasabi.repository.cassandra.pojo.index.UserAssignmentByUserId;
+import com.netflix.astyanax.Execution;


Do we need astyanax dependency here ?

No, we don't need this dependency... removed...

coveralls · 2017-01-09T22:32:33Z

Coverage increased (+0.02%) to 73.62% when pulling fffdf08 on feature/repository-datastax-migration-performance into 169002b on feature/repository-datastax-migration.

bargenilesh · 2017-01-09T22:44:01Z

Thanks @felixgao and @mans4singh for reviewing the code... I have addressed comments, kindly let me know if you have any further comments...

coveralls · 2017-01-09T23:29:30Z

Coverage increased (+0.02%) to 73.62% when pulling e36d662 on feature/repository-datastax-migration-performance into 169002b on feature/repository-datastax-migration.

coveralls · 2017-01-09T23:34:25Z

Coverage increased (+0.02%) to 73.62% when pulling e36d662 on feature/repository-datastax-migration-performance into 169002b on feature/repository-datastax-migration.

* Added initial impl of user feedback repo * updated comments * Added unit test for accessor * Impl + tests for user feedback repo * Updated test cases * pojo changes to use lombok * Audit log repository initial commit * Corrected bug and skip time since is checked during insert time * Updated test to create valid app name * Code cleanup and minor refactoring * Updated comments * Added integration tests * Set parsing log error to warn while retrieving data * Unit test for audit log repo * documentation + minor code correction * Documented issues * Updated logging * Initial impl for priorities repo * initial checkin for CassandraAuthorizationRepository and test * removed unused imports * add application list accessor and refactor cassandra authorization repsitory to use ApplicaitonList instead of plain String object * remove todo * refactor name app_name to appName * add binding to cassandra repository from interface * fix the badly refactored appName and add the missing UserDirectory module * fix the appName one more time * Added repo and unit/integration tests * add logging to log the keyspace it is using * Corrected column name * fix app_name refactor * Corrected comments * Added feedback repository intergration test * Updated comment * Updated comments * fix the application name cannot be * problem * First cut of mutex repo/accessor + integration tests * Added integration tests * Corrected test cases to take cassandra as argument * fix unit test fail * add integration test and more accessors. also fixed the local maven causes problem * refactor the naming and location of the sources * add more method to accessor * added page experiment index accessor * Added default and missing label * Corrected query and added simple integration test * Changed UserBucketIndexAccessor interface insert/delete to return void * add cassandra page repository using datastax. modified the original PageExperiment.java to include additional method in the builder * added additional accessor providers and fix cross import error and added page repository data provider for integration test * fix bad query in accessor and add additional test cases in repository module test * ExperimentAccessor + integration test for insert/get only * changed parameters to use proper java camel casing * add page respository integration test * Added methods and test cases for experiment and bucket accessors * Added method to get experiments * import list * Mutex repo updates + integration + unit tests * Updated comments and added tests * rename mutexaccssor to exclusionaccessor and add the mapping to guice module * add provider for all the accessors and remove some of the cross module dependencies mapping * Added integration tests for priorities repository * package base reordering * fix compile problem * Updated pom for objects dependency * fix bad import and missing interfaces * Application list accessor + integration tests * Integration tests for audit accessor + updates to experiment utils * Commented out astynax dependency from interface and database impl * refactor assignmnet builder to be proper builder and remove setters in original object. Also added AssignmentCountEnvelope.java that is missing from original repository * Added acessor + integration and unit test * Update notice and added comments * Updated guice module * fix bad table name * add some comment on existing code base and fixed accessor parameter type mismatch * fix type mismatch * Experiment repo + few integration tests * Added integration tests * added updateStateIndex back and minor documentation updates * updated get experiment list method * More integration tests * add missing methods for insert * Updated test case * Added more tests * removing testng integration test to a different directory * add missing deleteBy method * Added integration tests for experiment repo * initial repo migration for cassandra assignment repo, with incomplete code and unit test * Added ExperimentRuleCacheUpdateEnvelope * removed original repository/cassandra module and replaced with the new datastax module. the code compiles fine but have not tested for functional test * Updated module * added more unit test and fix some code issue with assignment repository * rearrange some accessors binding * fix accessor errors * Refactored tests * Corrected assignment-object test * fix guice error and depdency graph too complicated * Fixed tests cases * Corrected some comparisons to exclude description and rule for null vs empty string issue * Corrected issue with Optional.get() when the Optional is empty * fix optional get does not consider possible null error * Add exception handling for create exception - TODO - Validate experiment befor creation * fix assignment date is null bug * fix null assignment problem and left some todos for us to revisit soon * Updated validator and object setters for validation * Experiment validator test * Updated mock test cases * Updated integration and unit tests * Added unit tests * Updated test cases * Http status code cleanup in test * fix merge conflict issue * Removed redundant null check * Removed unused class * Commented out testng dependencies so that unit test execute * remove TODO * fix test case errors * fix the repository module dependency graph problem * Corrected mock tests * Updated test cases * fix the unit test * Added more tests cases * fix merge conflict and new features that are added to develop branch that is not in our feature branch * add missing mutation script * fix missing attributes from experiments and rename attribute to sneak case * Separated unit tests from integration tests * remove directly dependency of UserToolsModule from Repository module * add the migration script and reduce the file name for mutation script * Updated script * Created integration profile * Updated build script * mod: sync [ws]/bin with f/leroy-jenkins for jenkins/wasabi-internal parity * mod: rm bin stubs * add shell script and made migration part of the container.sh * add replication factor to migration script * added test case for adding to GET all assignments of an Invalid Experiment * update the migration to be using docker * set the client's remote host to zero to prefer localdc instead of remote * fix the local dc token aware policy to be able to connect to direct local dc * Commented out failing test * t_AuditLogPaginationSmoke should not use hardcoded username "admin. * added option to support feature. * Retrieve individual experiments to maintain priority order * added the ability to use the migration.sh without docker * Corrected check for response in case of experiment not found * Added override properties for database url, username and password * mod: add [ws]/bin/jenkins.sh * mod: u/d fpm.sh * mod: u/d * mod: u/d repo * mod: dbug fpm * mod: dbug fpm * mod: ui fpm * mod: rm fpm dbug * mod: bump release * Updated autumn version to 1.0.20160714060618. * refactor cassandra_client_config.properties to remove useless attributes and added more comment on what the new properties should be. Also added the default to pom.xml. * Feature/datastax-missingcases(getExpByApp,getExpByPage) (#128) * added test case for invalidapplication added util method in testbase * added test case for invalidapp and invalidpage * added test case for invalidapp pages * corrected the expectedstatus * Added database system properties to test base * Feature/table user_assignment_look_up rename and remove user_experiment_index references (#139) Renaming user assignment lookup and remove references for user experiment index. * Cleaned up changes for table name change (user assignment look up) * Updated table name * Removed commented code * Added attributions for utils * Renamed pojo and corrected clustering key * add the more debugging info for bad rule cache calls * fix a possible EOF error with keystore * add more logging info * fix datastax treat null values as empty string from db causes hyrule to throw exceptions * make PoolingOptions fully adjustable via configuration file * Removed lz4 1.1.0 dependency and commented astynax import from bucket class * add slow query logging and monitoring connection pooling and change logback to use async logger * Feature/repository datastax migration performance (#146) * Async Fetch of Experiment Metadata + custom performance logger + Minimize the impact of increase in # of experiments. * Added comments and logging variable replacements * Added uninterrupted implementations and unit tests.. * Self review: changed some comments and removed unused code.. * Addressed code review comments... * Added specific imports and fixed one functional condition... * Removed system.out from test class... * Refactored ApiModule so common module installation will not be repeated (#149) in child class and some modules installation can be overridden. * Resolved functional test issue and added exception catch and error message while connecting to cassandra... * 1. Changed version to 1.0.20161107232436-SNAPSHOT 2. Modified 2 props in cassandra_client_config.properties isSlowQueryLoggingEnabled:False, poolTimeoutMillis:0 3. disabled attribution-maven-plugin

bargenilesh added 4 commits December 29, 2016 14:18

Async Fetch of Experiment Metadata + custom performance logger + Mini…

d8602af

…mize the impact of increase in # of experiments.

Added comments and logging variable replacements

56a39c5

Added uninterrupted implementations and unit tests..

60e8a69

Self review: changed some comments and removed unused code..

3c03059

bargenilesh requested review from felixgao and mans4singh January 6, 2017 00:51

felixgao reviewed Jan 6, 2017

View reviewed changes

mans4singh reviewed Jan 9, 2017

View reviewed changes

Addressed code review comments...

fffdf08

bargenilesh added 2 commits January 9, 2017 15:20

Added specific imports and fixed one functional condition...

127a89b

Removed system.out from test class...

e36d662

bargenilesh merged commit 5d848a6 into feature/repository-datastax-migration Jan 10, 2017

bargenilesh deleted the feature/repository-datastax-migration-performance branch January 17, 2017 18:41

		@@ -45,6 +46,9 @@
		@Query("select * from bucket where experiment_id = ?")

		@@ -37,6 +38,9 @@
		@Query("select * from exclusion where base = ?")
		Result<Exclusion> getExclusions(UUID base);

		@@ -40,6 +41,10 @@
		@Query("select * from experiment_user_index where user_id = ? and app_name = ? and context = ?")
		Result<ExperimentUserByUserIdContextAppNameExperimentId> selectBy(String userId, String appName, String context);

		import com.intuit.wasabi.repository.cassandra.pojo.index.UserAssignmentByUserId;
		import com.netflix.astyanax.Execution;

Feature/repository datastax migration performance #146

Feature/repository datastax migration performance #146

Conversation

bargenilesh commented Jan 6, 2017

coveralls commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bargenilesh Jan 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 9, 2017

bargenilesh commented Jan 9, 2017

coveralls commented Jan 9, 2017

coveralls commented Jan 9, 2017

bargenilesh Jan 9, 2017 •

edited