Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DROOLS-7573 - add parameter for h2vm/infinispan store #268

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

nprentza
Copy link
Contributor

@nprentza nprentza marked this pull request as ready for review October 17, 2023 07:47
configureServicePriorities();
}else { // assuming infinispan is the default module
System.setProperty(DROOLS_RELIABILITY_MODULE_TEST, "INFINISPAN");
configureServicePriorities();
Copy link
Contributor

@tkobayas tkobayas Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this configureServicePriorities(); (for infinispan) to line-110. `configureServicePriorities() triggers storage initialization, so it should be called after all system properties are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

Comment on lines +69 to +72
public enum Module {
INFINISPAN,
H2MVSTORE
}
Copy link
Contributor

@tkobayas tkobayas Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought that Module is a good idea. But then we have to deal with unexpected combinations like "H2MVSTORE" + "REMOTE", or "H2MVSTORE" + "REMOTEPROTO".

If you run

java -jar target/drools-benchmarks-reliability.jar -jvmArgs "-Xms4g -Xmx4g" -foe true -wi 0 -i 1 -f 1 org.drools.benchmarks.reliability.InsertAndFireBenchmark

You will hit a failure with such a combination.

Probably it's better to use only Mode as a benchmark parameter. You may still use Module for internal code writing convenience (for example, Mode can have Module). So add "H2MVSTORE" to Mode. Maybe also it's better to rename like "EMBEDDED" to "INFINISPAN_EMBEDDED" and so on (but infinispanStorageMode values shouldn't be changed , because it's used for INFINISPAN_STORAGE_MODE)

Let me know if it's not clear for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I understand. I will make the changes needed.

@tkobayas
Copy link
Contributor

So overall, I recommend to run

java -jar target/drools-benchmarks-reliability.jar -jvmArgs "-Xms4g -Xmx4g" -foe true -wi 0 -i 1 -f 1 org.drools.benchmarks.reliability.InsertAndFireBenchmark

to check if benchmarks can run with all params combinations.

@nprentza
Copy link
Contributor Author

So overall, I recommend to run

java -jar target/drools-benchmarks-reliability.jar -jvmArgs "-Xms4g -Xmx4g" -foe true -wi 0 -i 1 -f 1 org.drools.benchmarks.reliability.InsertAndFireBenchmark

to check if benchmarks can run with all params combinations.

I run the above with all benchmarks, found a problem with InsertOnlyBenchmark and solved it by moving the call to configureServicePriorities() before attempting to instantiate the InfinispanStorageManager (class AbstractReliabilityBenchmark, method setupEnvironment() ).

@mariofusco mariofusco self-requested a review October 19, 2023 08:09
Copy link
Contributor

@tkobayas tkobayas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mariofusco mariofusco merged commit 0ddde0a into apache:main Oct 19, 2023
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants