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

CHINA-280: Set 1 hour request timeout for S3 during restore #154

Merged
merged 5 commits into from
May 29, 2024

Conversation

ianton-ru
Copy link
Member

No description provided.

@ianton-ru ianton-ru force-pushed the CHINA-280 branch 8 times, most recently from 94cb7cb to b67ffc0 Compare May 24, 2024 13:45
@ianton-ru
Copy link
Member Author

Failed tests look like docker/docker-py#3256

@MikhailBurdukov
Copy link
Contributor

Failed tests look like docker/docker-py#3256

Fix in #153

ch_backup/clickhouse/disks.py Outdated Show resolved Hide resolved
Comment on lines 147 to 159

request_timeout_ms = int(disk_config.get("request_timeout_ms", 0))
if request_timeout_ms < CH_OBJECT_STORAGE_REQUEST_TIMEOUT_MS:
disks_config[tmp_disk_name]["request_timeout_ms"] = str(
CH_OBJECT_STORAGE_REQUEST_TIMEOUT_MS
)
disks_config[disk_name] = {
"request_timeout_ms": {
"@replace": "replace",
"#text": str(CH_OBJECT_STORAGE_REQUEST_TIMEOUT_MS),
}
}

Copy link
Contributor

@MikhailBurdukov MikhailBurdukov May 24, 2024

Choose a reason for hiding this comment

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

have tested manualy and:

root@clickhouse01:/# cat /tmp/clickhouse-disks-config.xml 
<?xml version="1.0" encoding="utf-8"?>
<yandex>
        <storage_configuration>
                <disks>
                        <hdd1>
                                <path>/hdd1/</path>
                        </hdd1>
                        <hdd2>
                                <path>/hdd2/</path>
                        </hdd2>
                        <s3>
                                <type>s3</type>
                                <endpoint>http://minio01:9000/cloud-storage-01/data/</endpoint>
                                <access_key_id>***</access_key_id>
                                <secret_access_key>**</secret_access_key>
                        </s3>
                        <s3_source>
                                <type>s3</type>
                                <endpoint>http://minio01:9000/cloud-storage-01/data/</endpoint>
                                <access_key_id>**</access_key_id>
                                <secret_access_key>**</secret_access_key>
                        </s3_source>
                </disks>
        </storage_configuration>

Is that exepected?

Ps. Im not sure if the test runned correctly due to broken CI, will recheck after green CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't understand. ch-backup doesn't modify original config files, it creates new one with name cloud_storage_tmp_disk_<disk_name>_source.xml. So clickhouse-disks-config.xml - it's an original file, not created by ch-backup.

Copy link
Contributor

@MikhailBurdukov MikhailBurdukov May 27, 2024

Choose a reason for hiding this comment

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

/tmp/clickhouse-disks-config.xml not /var/lib/clcikhouse/... We are generating additional config for clickhouse-disks usage. But I think we don't have to worry that as long as CI is broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

ianton-ru and others added 2 commits May 27, 2024 11:22
Co-authored-by: MikhailBurdukov <102754618+MikhailBurdukov@users.noreply.github.com>
Comment on lines +153 to +157
disks_config[disk_name] = {
"request_timeout_ms": {
"@replace": "replace",
"#text": str(CH_OBJECT_STORAGE_REQUEST_TIMEOUT_MS),
}
Copy link
Contributor

@MikhailBurdukov MikhailBurdukov May 29, 2024

Choose a reason for hiding this comment

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

What is the motivation of replacing request_timeout_ms for clickhouse-server config? AFAIK there is only one long-running operation(CopyObject) and is it done using with clickhouse-disk. Maybe make sense to keep request_timeout_ms only for clickhouse-disks config?

Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency.
If source disk is not used in clickhouse-server, better remove additional config for server at all.

@ianton-ru ianton-ru merged commit 4f20a8d into main May 29, 2024
13 of 14 checks passed
@ianton-ru ianton-ru deleted the CHINA-280 branch May 29, 2024 15:30
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.

None yet

2 participants