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

Introduce session setup #422

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ranaalotaibiMS
Copy link
Contributor

@ranaalotaibiMS ranaalotaibiMS commented Dec 12, 2023

This PR introduces functionality for configuring a session by executing predefined statements before the benchmark execution begins. Users should specify the file containing these statements within the tag <sessionsetupfile> </sessionsetupfile> in the benchmarks' config files. CC: @bpkroth

For example,

<?xml version="1.0"?>
<parameters>

    <!-- Connection details -->
    <type>sqlserver</type>
...
    <!-- Session setup statements file -->
    <sessionsetupfile>config/sqlserver/session_setup_sqlserver_cmds_example.sql</sessionsetupfile>
...

@apavlo
Copy link
Member

apavlo commented Dec 12, 2023

I like this a lot but I don't think the files should go into data since we're removing it in #228.
This is actually related to #420 where we need to have DBMS-specific configurations.

@bpkroth
Copy link
Collaborator

bpkroth commented Dec 14, 2023

I like this a lot but I don't think the files should go into data since we're removing it in #228. This is actually related to #420 where we need to have DBMS-specific configurations.

Agree with you on this one - it's DB specific so can/should just go in the DB's config directory.

However, there are a few places where we do have non-DB specific files in that data directory (e.g., templated example config).

Could just duplicate them or find another place to put them.

@bpkroth
Copy link
Collaborator

bpkroth commented Dec 14, 2023

@ranaalotaibiMS , #416 and other recent PRs introduced a lot of changes, especially around standardizing code formatting, so you'll have to rebase your changes on an updated main branch first.

@ranaalotaibiMS
Copy link
Contributor Author

ranaalotaibiMS commented Jan 3, 2024

@ranaalotaibiMS , #416 and other recent PRs introduced a lot of changes, especially around standardizing code formatting, so you'll have to rebase your changes on an updated main branch first.

@bpkroth : Rebased

@ranaalotaibiMS ranaalotaibiMS marked this pull request as ready for review January 3, 2024 02:02
@@ -10,6 +10,8 @@
<reconnectOnConnectionFailure>true</reconnectOnConnectionFailure>
<isolation>TRANSACTION_SERIALIZABLE</isolation>
<batchsize>1024</batchsize>
<!-- Session setup statements file -->
<!-- <sessionsetupfile>config/sqlserver/session_setup_sqlserver_cmds_example.sql</sessionsetupfile> -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Request that we have at least one example config where this option is not commented out so that it runs during the CI tests and/pr add an explicit unit test for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ranaalotaibiMS bump :)

@bpkroth
Copy link
Collaborator

bpkroth commented Jan 23, 2024

@ranaalotaibiMS, seems like there are some missing formatting changes. Can you please address those along with the requested example config for testing changes? Thanks!

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