rework olap_workload #12870
rework olap_workload #12870
Conversation
f43bb32 to
6ca36e4
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
ydb/tools/olap_workload/__main__.py
Outdated
| if self.use_query_service(): | ||
| return self.session_pool.execute_with_retries(statement) | ||
| else: | ||
| if ddl: |
There was a problem hiding this comment.
Мне кажется какой-то нестройный подход: типа я бы понял, если бы ddl полностью поддерживался в этой функции, но у тебя появилась функция "drop_table".
Может проще было бы удалить аргумент ddl из этого метода и просто завести отдельный метод create_table? Оно хотя бы расширяемо будет
There was a problem hiding this comment.
Не уверен, что это нужно. Поэтому пока оставил TODO
|
|
||
| def _generate_new_table_n(self): | ||
| while True: | ||
| r = random.randint(1, 40000) |
There was a problem hiding this comment.
А зачем числа? Мне кажется проще использовать random_string(..)
There was a problem hiding this comment.
Мне с числами удобнее. Их проще держать в голове при отладке
There was a problem hiding this comment.
Я не возражаю иметь числовые идентификаторы, но ты в self.tables все таки строки храни имхо
| n = self._get_existing_table_n() | ||
| if n is None: | ||
| print("create_drop: No tables to delete") | ||
| time.sleep(10) |
There was a problem hiding this comment.
Мне кажется слип слишком большой, наверное мы не хотим такие большие тормоза на джоине треда видеть, предлагаю написать 0.1 например
There was a problem hiding this comment.
Тут идея была в том, чтобы накопить какое-то кол-вон созданных таблиц, когда их кол-во снизилось до 0
| n = self._generate_new_table_n() | ||
| self.create_table(str(n)) | ||
| with self.lock: | ||
| self.tables.add(n) |
There was a problem hiding this comment.
А зачем в self.tables хранить какие то числа, а не просто пути до таблиц? Предлагаю просто строчки хранить
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢 |
| print("create_drop: No tables to delete") | ||
| time.sleep(10) | ||
| continue | ||
| self.client.drop_table(self.get_table_path(str(n))) |
There was a problem hiding this comment.
У тебя как будто не проверяются фэйлы нигде
Если дроп тэйбл сфэйлит, то тред просто умрет и перестанет работать
Посмотри как сделано в simple_queue: там все ошибки попадают в стату, мне кажется надо сделать так же в get_stat (видимо код переиспользовать)
Я просто подозреваю, что по причине отсутствия обработки ошибок ворклоад проработает условные полчаса и перестанет генерить запросы
There was a problem hiding this comment.
Я исхожу из того, что все ошибки должны ретраится SDK. Если это не помогло, то тест должен упасть
There was a problem hiding this comment.
Я некоторое время думал про это, и пришел к выводу, что кажется, если хочешь падать, то мне кажется лучше это сделать через отдельную опцию
В текущем состоянии мне кажется это не требуется, пусть это будет следующим шагом
There was a problem hiding this comment.
И как будто все равно нет перехвата исключений
ydb/tools/olap_workload/__main__.py
Outdated
| workload.run() | ||
| client = YdbClient(args.endpoint, args.database, True) | ||
| client.wait_connection() | ||
| with WorkloadRunner(client, "olap_workload", args.duration) as runner: |
There was a problem hiding this comment.
Посмотри как сделано в simple_queue: там зашивается имя хоста в префикс пути таблицы, мне кажется тут так же надо сделать, чтобы разные запуски ворклоадов не пересекались
There was a problem hiding this comment.
вынес в аргументы командной строки. При желании можно разделять разные запуски на одной базе
ydb/tools/olap_workload/__main__.py
Outdated
| self.select_n(table_name, 300) | ||
| actual = self.client.query( | ||
| f""" | ||
| select count(*) as cnt, sum(i64Val) as vals, sum(id) as ids FROM `{table_path}` |
There was a problem hiding this comment.
Может все таки в upper case как в других местах?
ydb/tools/olap_workload/__main__.py
Outdated
| )[0].rows[0] | ||
| expected = {"cnt": i, "vals": i * (i + 1) * 5, "ids": i * (i + 1)} | ||
| if actual != expected: | ||
| raise f"Incorrect result: expected:{expected}, actual:{actual}" |
There was a problem hiding this comment.
Оно рейзится и не обрабатывается, после чего тред просто умирает, кажется не должно так работать
There was a problem hiding this comment.
Добавил перехват исключений в код базового класча
ydb/tools/olap_workload/__main__.py
Outdated
| parser.add_argument('--batch_size', default=1000, help='Batch size for bulk insert') | ||
| parser.add_argument('--endpoint', default='localhost:2136', help="An endpoint to be used") | ||
| parser.add_argument('--database', default='Root/test', help='A database to connect') | ||
| parser.add_argument('--duration', default=10, type=lambda x: int(x), help='A duration of workload in seconds.') |
There was a problem hiding this comment.
Предлагаю дефолтный duration оставить как было: 10 ** 9
| def query(self, statement, is_ddl): | ||
| if self.use_query_service: | ||
| return self.session_pool.execute_with_retries(statement) | ||
| else: |
There was a problem hiding this comment.
Синтаксическая красивость, оператор elif.
There was a problem hiding this comment.
Я про нею знаю, но в данном случае, мне нравится больше этот спсобо, т.к. он структурно более выразительный
| def _remove_recursively(self, path): | ||
| deleted = 0 | ||
| d = self.driver.scheme_client.list_directory(path) | ||
| for entry in d.children: | ||
| entry_path = "/".join([path, entry.name]) | ||
| if entry.is_directory(): | ||
| deleted += self._remove_recursively(entry_path) | ||
| elif entry.is_column_table() or entry.is_table(): | ||
| self.drop_table(entry_path) | ||
| deleted += 1 | ||
| else: | ||
| raise f"Scheme entry {entry_path} of unexpected type" | ||
| self.driver.scheme_client.remove_directory(path) | ||
| return deleted | ||
|
|
||
| def remove_recursively(self, path): | ||
| d = self.describe(path) | ||
| if d is None: | ||
| return | ||
| if not d.is_directory(): | ||
| raise f"{path} has unexpected type" | ||
| return self._remove_recursively(path) |
There was a problem hiding this comment.
Есть ощущение, что это у нас написано уже в нескольких местах. Например вот тут. Предлагаю переиспользовать. Возможно это вообще затащить в sdk.
There was a problem hiding this comment.
Тут весь раннер - это переписывание аналогов. Нужно бы их как-то смёрджить. Но не в рамках этого PR
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
Conflicts: ydb/tools/olap_workload/__main__.py
Conflicts: ydb/tools/olap_workload/__main__.py
Changelog entry
Load test for column tables
...
Changelog category
Additional information
...