SO-3125: Resolve validation performance issues #246

zoliMolnar · 2018-06-29T07:29:27Z

https://snowowl.atlassian.net/browse/SO-3125

This pull request resolves validation performances by utilizing the jobs API, to async run multiple validation rules.

... implementations to make use of the eclipse job API

... the executor service

... "end" in ValidateRequest changed isConflicting logic in ValidationRuleSchedulingRule, added tests to test ValidationThreadPool

... returned with not OK status

... rule validation

issue/SO-3125_resolve_validation_performance_issues Conflicts: snomed/com.b2international.snowowl.validation.snomed/src/main/resources/scripts/ruleSnomedCommon1.groovy snomed/com.b2international.snowowl.validation.snomed/src/main/resources/scripts/ruleSnomedCommon2.groovy

cmark · 2018-06-29T07:39:03Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

@@ -0,0 +1,170 @@
+/*
+ * Copyright 2011-2018 B2i Healthcare Pte Ltd, http:https://b2i.sg


New files should indicate current year as starting year in the license header.

cmark · 2018-06-29T07:41:35Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

+ */
+public class ValidationThreadPoolTest {
+
+ private static final int MAXIMUM_AMOUNT_OF_RUNNING_EXPENSIVE_JOBS = 1;


Provide this as configuration to the ValidationThreadPool class so deployments with more computational power (CPU, RAM, ES cluster nodes) can increase the number of expensive jobs. This would be useful for the other CheckTypes as well.

cmark · 2018-06-29T07:44:27Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

+
+ @Test
+ public void testConcurrentExpensiveJobs() {
+ final ValidationThreadPool pool = new ValidationThreadPool(VALIDATION_THREAD_COUNT);


Test fixture initialization should go into a @Before method, please convert common variables to fields and initialize them in that method.

cmark · 2018-06-29T07:45:05Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

+
+ final List<Promise<Object>> validationPromises = Lists.newArrayList();
+ for (int i = 0; i < 10; i++) {
+ validationPromises.add(pool.submit(CheckType.NORMAL, createValidatableRunnable(CheckType.FAST, manager)));


⚠️ NORMAL vs FAST!

cmark · 2018-06-29T07:46:20Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

+ if (CheckType.NORMAL == checkType) {
+ assertTrue(runningNormalJobs <= MAXIMUM_AMOUNT_OF_RUNNING_NORMAL_JOBS);
+ }
+ Thread.sleep(500);


Would be nice to configure this hardcoded 500 ms timeout value based on the given CheckType, just to simulate the cost more accurately.

cmark · 2018-06-29T08:06:19Z

...e/src/com/b2international/snowowl/core/internal/validation/ValidationRuleSchedulingRule.java

+
+ @Override
+ public int hashCode() {
+ int hash = 17;


Use Objects.hash(args) correctly, where you pass all values directly to the function and not one-by-one.

cmark · 2018-06-29T08:33:40Z

...e/src/com/b2international/snowowl/core/internal/validation/ValidationRuleSchedulingRule.java

+
+ @Override
+ public String toString() {
+ return uniqueRuleId;


Add checkType to toString()

cmark · 2018-06-29T08:34:32Z

...datastore/src/com/b2international/snowowl/core/internal/validation/ValidationThreadPool.java

+ job.setRule(schedulingRule);
+ job.addJobChangeListener(new JobChangeAdapter() {
+
+ public void done(IJobChangeEvent event) {


Missing @Override annotation.

cmark · 2018-06-29T08:36:41Z

...datastore/src/com/b2international/snowowl/core/internal/validation/ValidationThreadPool.java

+ final Job job = new ValidationJob(checkType.getName(), runnable);
+ String uniqueRuleId = UUID.randomUUID().toString();
+ final ISchedulingRule schedulingRule = new ValidationRuleSchedulingRule(checkType, maxValidationThreadCount, uniqueRuleId);
+ final Promise<Object> isRuleDonePromise = new Promise<>();


Rename isRuleDonePromise to just promise.

cmark · 2018-06-29T08:37:44Z

...astore/src/com/b2international/snowowl/core/validation/rule/ValidationRuleCreateRequest.java

@@ -44,6 +41,8 @@
 @NotNull 
 private Severity severity;

+ private CheckType checkType;


Add NotNull annotation or default value CheckType.NORMAL.

... cancel status - Add check against null values for family and runnables, change belongsTo to use Objects.equal - Add NotNull annotaion to ValidationRuleCreateRequest - Add configurability to limit the maximum amount of concurrent normal or expensive jobs to ValidationBootstrap, ValidationThreadPool, ValidationRuleSchedulingRule - Fix test class and added logging for test

... fetched concepts in SnomedValidationIssueDetailExtension.class, added validation rule execution time logging to ValidateRequest.class

…validation_performance_issues

...in case of _source = false configuration This will reduce the memory consumption of aggregations with top hits somewhat.

cmark · 2018-07-04T09:39:29Z

...e.server.tests/src/com/b2international/snowowl/core/validation/ValidationThreadPoolTest.java

@@ -54,13 +67,12 @@ public void testConcurrentExpensiveJobs() {
 }

 Promise.all(validationPromises).getSync();
+ System.out.println();


Remove println calls. If you would like to separate the log output of the test cases then use a JUnit Rule like this class: com.b2international.snowowl.test.commons.TestMethodNameRule.

cmark · 2018-07-04T09:40:53Z

....datastore/src/com/b2international/snowowl/core/internal/validation/ValidationBootstrap.java

@@ -78,7 +78,11 @@ public void preRun(SnowOwlConfiguration configuration, Environment env) throws E
 // initialize validation thread pool
 // TODO make this configurable
 int numberOfValidationThreads = Math.max(2, Runtime.getRuntime().availableProcessors() / 2);
- env.services().registerService(ValidationThreadPool.class, new ValidationThreadPool(numberOfValidationThreads));
+ // TODO calculate these based on systems overall strength


Move these to actual snowowl_config.yml configuration values.

cmark · 2018-07-04T09:41:25Z

...nowowl.datastore/src/com/b2international/snowowl/core/internal/validation/ValidationJob.java

@@ -48,7 +60,7 @@ protected IStatus run(IProgressMonitor monitor) {

 @Override
 public boolean belongsTo(Object family) {
- return (this.family == null || family == null) ? false : this.family.equals(family);
+ return Objects.equal(this.family, family);


Use java.util.Objects, please.

cmark · 2018-07-04T09:43:01Z

...e/src/com/b2international/snowowl/core/internal/validation/ValidationRuleSchedulingRule.java

- hash = hash * 31 + Objects.hash(checkType);
- hash = hash * 31 + maxConcurrentJobs;
- hash = hash * 31 + Objects.hash(uniqueRuleId); 
+ hash = hash * 31 + Objects.hash(checkType, id, maxConcurrentJobs, maxConcurrentExpensiveJobs);

 return hash;


Just return the value computed by the Objects.hash method, like this:

@Override public int hashCode() { return Objects.hash(checkType, id, maxConcurrentJobs, maxConcurrentExpensiveJobs); }

cmark · 2018-07-04T09:43:17Z

...e/src/com/b2international/snowowl/core/internal/validation/ValidationRuleSchedulingRule.java

 }

 @Override
 public String toString() {
- return uniqueRuleId;
+ return id + " " + checkType.getName();


Use String.format() method.

... and expensive validation jobs configurable

... and persisting them again

... to separate tests in the log

... computed value in ValidationRuleSchedulingRule hashCode

cmark

LGTM!

zoliMolnar · 2018-07-04T13:42:22Z

Thank you!

Zoltan Molnar added 11 commits June 27, 2018 18:19

SO-3125: Add missing package declarations to rules

b8e2664

SO-3125: Add checkType property to validation-rules-common rules

f011b1a

SO-3125: Add checkType enum and new property to ValidationRule

a3ebaf5

SO-3125: Added custom ValidationJob and ValidationRuleSchedulingRule...

75b3a6f

... implementations to make use of the eclipse job API

SO-3125: Refactor ValidationThreadPool to use the Job API instead of ...

4c2bd93

... the executor service

SO-3125: Added Promise.all to ValidateRequest to wait for every job's...

1eab4cb

... "end" in ValidateRequest changed isConflicting logic in ValidationRuleSchedulingRule, added tests to test ValidationThreadPool

SO-3125: Modified logic of isConflicting in ValidationRuleSchedulingRule

88b4759

SO-3125: Added rejection to promise if the job status after finishing...

716d7f7

... returned with not OK status

SO-3125: Added test cases for most use-case functionalities

8c74d31

SO-3125: Reduce the limit to 3 from 4 of medium checkType validation...

63dde07

... rule validation

SO-3125: Removed extra lines

43fe605

zoliMolnar self-assigned this Jun 29, 2018

zoliMolnar requested a review from cmark June 29, 2018 07:29

cmark requested changes Jun 29, 2018

View reviewed changes

Zoltan Molnar and others added 11 commits June 29, 2018 18:28

SO-3125: Fix test classes

6ee5be6

SO-3125: Refactor aggregation in ruleSnomedCommon2 to use fields

ce3cb8b

SO-3125: Increase scroll limit to 50_000, prevent fetching of already...

3793f29

... fetched concepts in SnomedValidationIssueDetailExtension.class, added validation rule execution time logging to ValidateRequest.class

SO-3125: Remove unused imports

b00a5e8

Merge remote-tracking branch 'origin/6.x' into issue/SO-3125_resolve_…

4037227

…validation_performance_issues

Merge remote-tracking branch 'origin/6.x' into issue/SO-3125_resolve_…

a3dac66

…validation_performance_issues

SO-3125: reduce memory consumption of SnomedQueryValidationRuleEvaluator

b6ccdc4

SO-3125: use a queue to persist issues generated so far by workers

9e35a24

SO-3125: configure top hits aggregation to skip stored field loading...

a59cb7e

...in case of _source = false configuration This will reduce the memory consumption of aggregations with top hits somewhat.

SO-3125: improve validate request log messages

cc3b8fc

cmark requested changes Jul 4, 2018

View reviewed changes

Zoltan Molnar added 3 commits July 4, 2018 11:54

SO-3125: Make the amount of validation threads, concurrent normal...

09c971e

... and expensive validation jobs configurable

SO-3125: Make validation work with issue deltas instead of dropping...

30d92ad

... and persisting them again

SO-3125: Use String.format in toString

1a25c48

Zoltan Molnar and others added 3 commits July 4, 2018 13:05

SO-3125: Remove printlns from test use ValidationTestMethodNameRule...

fdc6b53

... to separate tests in the log

SO-3125: Use java.util.Objects in ValidationJob return Objects.hash...

2843211

... computed value in ValidationRuleSchedulingRule hashCode

SO-3125: use Stopwatch instead of custom elapsed time tracking

77db82d

cmark approved these changes Jul 4, 2018

View reviewed changes

cmark merged commit de60e7f into 6.x Jul 4, 2018

cmark deleted the issue/SO-3125_resolve_validation_performance_issues branch July 4, 2018 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SO-3125: Resolve validation performance issues #246

SO-3125: Resolve validation performance issues #246

zoliMolnar commented Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jun 29, 2018

cmark Jul 4, 2018

cmark Jul 4, 2018

cmark Jul 4, 2018

cmark Jul 4, 2018

cmark Jul 4, 2018

cmark left a comment

zoliMolnar commented Jul 4, 2018

		@@ -0,0 +1,170 @@
		/*
		* Copyright 2011-2018 B2i Healthcare Pte Ltd, http:https://b2i.sg

SO-3125: Resolve validation performance issues #246

SO-3125: Resolve validation performance issues #246

Conversation

zoliMolnar commented Jun 29, 2018

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

cmark left a comment

Choose a reason for hiding this comment

zoliMolnar commented Jul 4, 2018