Skip to content

Configuration Refactor #134

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

Draft
wants to merge 100 commits into
base: main
Choose a base branch
from
Draft

Conversation

timveil
Copy link
Collaborator

@timveil timveil commented Mar 14, 2022

This PR contains a number of changes related to a significant refactor of configuration xml files and their associated classes. The changes largely fall into the following buckets...

  • separate existing config file into three separate files... database, workload and dialect. database contains only configuration about the target database. workload contains only information about the benchmark or workload. dialect contains only statement level overwrites.
  • each new config file is now validated with a new .xsd file
  • xpath and other legacy jars/frameworks have been removed in favor of Jackson Xml for mapping of xml documents to Java Objects.
  • the Java representation of the configuration now uses the new record java type

The net result of these changes among other things is a simplification of the logic in DBWorkload.

In addition to the above changes, another fix was inadvertently added to this PR. The wikipedia benchmark incorrectly threw 100% aborted transactions for 3 procedures. This issue, #122, is resolved as part of this PR

@@ -187,7 +182,7 @@ public int chooseTransaction(boolean isColdQuery) {
for (int i = 0; i < this.weightCount; i++) {
weight += weights.get(i);
if (randomPercentage <= weight) {
return i + 1;
return i;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a bugfix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without this change the workload would not run at times. i would get an index out of bounds exception. This makes sense because the value returned should be an index to a collection which is zero based. In other words if weightCount is say 2, when i = 1 the resulting return value you would be 1+1 = 2. 2 is not a valid index of collection who's length is 2.

but truthfully i don't really understand what this method is attempting to do. part of my refactor was to rename these methods to more clearly express what they seemed to be doing in practice. For example this method was renamed from chooseTransaction to chooseTransactionIndex because that more clearly communicated to me the intent of the function as i understood it. I could use some help here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there might be something wrong here... i need to do some more research

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i've broken something here that needs to be fixed before this PR can be accepted. I need some help to better understand phases. I think i've broken the link between weight and transaction by introducing transactionid collision. its seems like we could simplify things here but i'll need some guidance

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably won't be able to take a more detailed pass on this until the weekend

Copy link
Collaborator

@lmwnshn lmwnshn left a comment

Choose a reason for hiding this comment

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

Personal checkpoint for the day; have skimmed everything except these five files which have more substantial changes.

DBWorkload
WorkloadConfiguration
BenchmarkModule
StatementDialects
TPCDSLoader

Overall LGTM so far, nice refactor!

@timveil timveil added in-progress This PR is in progress. and removed ready-for-review This PR is ready to be reviewed. labels Mar 18, 2022
# Conflicts:
#	.github/workflows/maven.yml
#	config/cockroachdb/sample_tpch_config.xml
#	config/mariadb/sample_tpch_config.xml
#	config/mysql/sample_tpch_config.xml
#	config/postgres/sample_tpch_config.xml
#	docker/tpch/README.md
#	src/main/java/com/oltpbenchmark/DBWorkload.java
#	src/main/java/com/oltpbenchmark/WorkloadConfiguration.java
#	src/main/java/com/oltpbenchmark/api/BenchmarkModule.java
#	src/main/java/com/oltpbenchmark/api/TransactionTypes.java
#	src/main/java/com/oltpbenchmark/benchmarks/tpch/TPCHLoader.java
#	src/main/java/com/oltpbenchmark/benchmarks/voter/VoterBenchmark.java
#	src/main/java/com/oltpbenchmark/util/Histogram.java
#	src/main/resources/benchmarks/epinions/ddl-generic.sql
#	src/main/resources/benchmarks/epinions/dialect-oracle.xml
#	src/main/resources/benchmarks/epinions/dialect-sqlserver.xml
#	src/main/resources/benchmarks/tatp/ddl-generic.sql
#	src/main/resources/benchmarks/tpcc/ddl-generic.sql
#	src/main/resources/benchmarks/twitter/ddl-generic.sql
#	src/main/resources/benchmarks/wikipedia/ddl-generic.sql
#	src/test/java/com/oltpbenchmark/api/AbstractTestBenchmarkModule.java
#	src/test/java/com/oltpbenchmark/api/AbstractTestCase.java
#	src/test/java/com/oltpbenchmark/api/AbstractTestLoader.java
#	src/test/java/com/oltpbenchmark/api/AbstractTestWorker.java
#	src/test/java/com/oltpbenchmark/api/MockBenchmark.java
#	src/test/java/com/oltpbenchmark/benchmarks/auctionmark/TestAuctionMarkLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/auctionmark/TestAuctionMarkWorker.java
#	src/test/java/com/oltpbenchmark/benchmarks/seats/TestSEATSWorker.java
#	src/test/java/com/oltpbenchmark/benchmarks/smallbank/TestSmallBankLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/tatp/TestTATPLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/tpcc/TestTPCCLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/tpch/TestTPCHLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/twitter/TestTwitterLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/voter/TestVoterLoader.java
#	src/test/java/com/oltpbenchmark/benchmarks/wikipedia/TestWikipediaLoader.java
#	src/test/java/com/oltpbenchmark/catalog/TestCatalog.java
#	src/test/java/com/oltpbenchmark/util/TestTableDataIterable.java
#	src/test/resources/log4j.properties
@viktor-51
Copy link

viktor-51 commented Apr 23, 2022

Hi I have tried your code out and I seem to have a bug using Wikipedia. When i load the database with PostgreSQL as;

java -jar benchbase.jar -w config/workload/tpcc.xml -d config/db/postgres.xml --create=true --load=true

Then execute as;

java -jar benchbase.jar -w config/workload/tpcc.xml -d config/db/postgres.xml --execute=true

Then the database just freezes at;

[INFO ] 2022-04-23 14:38:53,269 [main] com.oltpbenchmark.ThreadBench runRateLimitedMultiPhase - TERMINATE :: Waiting for all terminals to finish ..

But when i run the database as;

java -jar benchbase.jar -w config/workload/tpcc.xml -d config/db/postgres.xml --create=true --load=true --execute=true

Then it works fine.

Best regards Viktor Olsson :)

@apavlo apavlo marked this pull request as draft June 10, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress This PR is in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aborted transactions in Wikipedia
3 participants