-
Notifications
You must be signed in to change notification settings - Fork 7
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
Parallel FREEZE TABLE #164
Conversation
It looks like we encountered a bug(or bugs) here described here Delgan/loguru#231 for Python 3.6. I have added printing of stack trace of threads upon getting SIGTERM
Traceback:
|
ch_backup/ch_backup.py
Outdated
@@ -48,7 +48,9 @@ def __init__(self, config: Config) -> None: | |||
self._config = config | |||
self._access_backup_manager = AccessBackup() | |||
self._database_backup_manager = DatabaseBackup() | |||
self._table_backup_manager = TableBackup() | |||
self._table_backup_manager = TableBackup( | |||
self._config.get("multiprocessing").get("freeze_workers", 1) |
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.
It makes sense use get()
only when a default is expected. The error of absent key for []
is more readable.
Now tables are freezed before backup is started, maybe we should raise default number of freeze threads |
@@ -474,15 +475,27 @@ def system_unfreeze(self, backup_name: str) -> None: | |||
query_sql = SYSTEM_UNFREEZE_SQL.format(backup_name=backup_name) | |||
self._ch_client.query(query_sql, timeout=self._unfreeze_timeout) | |||
|
|||
def remove_freezed_data(self) -> None: | |||
def remove_freezed_data( | |||
self, backup_name: Optional[str] = None, table: Optional[Table] = None |
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.
Let's introduce a new object with backup_name and table as members. Or add checking and throwing an exception when only one from the pair is provided.
@@ -776,6 +789,26 @@ def chown_dir(self, dir_path: str) -> None: | |||
need_recursion, | |||
) | |||
|
|||
def create_shadow_increment(self) -> None: | |||
""" | |||
Create shadow/increment.txt to fix race condition with parallel freeze. |
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 there some issue or comment in the code of CH about this to mention here ?
ch_backup/logic/table.py
Outdated
table.name, | ||
) | ||
|
||
table_freezer.freeze_table( |
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.
Let's do that simpler.
Just pass list of tables to freezer to the synchronous method freeze_tables
(or make it as function), returning after all is frozen.
Without accumulating, waiting and context manager.
4 is OK, for starter |
No description provided.