-
Notifications
You must be signed in to change notification settings - Fork 1
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
Baseline Experiments. #345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @amlatyrngom. Can you test this out on the 100 GB dataset with the scale down workload? experiments/15-e2e-scenarios-v2/scale_down/run_workload.sh
. The script assumes it is being run by a tool called Conductor. The easiest way to run it without the tool for testing is to set the COND_OUT
environment variable to a path where you want to store the results.
@pytest.mark.skip( | ||
reason="TODO(Amadou): This is failing even I haven't changed it. Flaky test?" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be fixed now - you can remove this decorator and the test should pass.
@@ -0,0 +1,100 @@ | |||
# See workloads/cross_db_benchmark/benchmark_tools/tidb/README.md | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this file into workloads/IMDB_extended
cursor = self._cursor | ||
# Exec | ||
cursor.execute(query) | ||
if cursor.rowcount is None or cursor.rowcount <= 0 or not(query.strip().lower().startswith("SELECT")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query.strip().lower().startswith("SELECT")
is always going to be false?
# Get cursor | ||
if self._cursor is None: | ||
had_cursor = False | ||
cursor = self._conn.cursor() | ||
else: | ||
had_cursor = True | ||
cursor = self._cursor | ||
# Exec | ||
cursor.execute(query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes to support a MySQL connector? If so, please just create another Database
class that is specialized for MySQL instead of modifying this class.
if cursor.rowcount is None or cursor.rowcount <= 0 or not(query.strip().lower().startswith("SELECT")): | ||
rows = [] | ||
else: | ||
print(f"Rows: {cursor.rowcount}. Q: {query}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please use logger.debug()
instead of print()
. Otherwise this will get very verbose during a workload run.
dataset_type = "20gb" | ||
else: | ||
dataset_type = "original" | ||
worker = TransactionWorker(worker_idx, args.seed ^ worker_idx, args.scale_factor, dataset_type=dataset_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - thanks for catching this. Instead of setting dataset_type
here based on flags, I'd suggest we make it an argparse argument instead.
parser.add_argument( | ||
"--scale-factor", | ||
type=int, | ||
default=1, | ||
default=6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please keep this at 1 for now.
if __name__ == "__main__": | ||
yaml_main() | ||
sys.exit(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed?
parser.add_argument( | ||
"--output-dir", | ||
type=str, | ||
default=".", | ||
help="Environment variable that stores the output directory of tidb bench", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still used? Also it seems like the help string is not correct since the default is a path and not an environment variable?
662f55c
to
07f634d
Compare
Closing since I think the intention was to not merge given the code divergence. |
No description provided.