Skip to content

[bitnami/clickhouse] Stops unnecessary logging #32988

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

Merged
merged 7 commits into from
Apr 15, 2025

Conversation

ilpianista
Copy link
Contributor

Description of the change

Disable Clickhouse logs which are used to analyze the details of query performance.

Benefits

Decrease clickhouse disk usage.

Possible drawbacks

Users should enable these logs when needed.

Applicable issues

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added clickhouse triage Triage is needed labels Apr 14, 2025
@github-actions github-actions bot requested a review from javsalgar April 14, 2025 09:32
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Apr 14, 2025
@github-actions github-actions bot removed the triage Triage is needed label Apr 14, 2025
@github-actions github-actions bot removed the request for review from javsalgar April 14, 2025 10:28
@github-actions github-actions bot requested a review from juan131 April 14, 2025 10:28
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Hi @ilpianista

Thanks so much for the contribution! Given we're planning important changes at #32958, I'll block this PR until the mentioned changes are landed.

@ilpianista
Copy link
Contributor Author

Thanks so much for the contribution! Given we're planning important changes at #32958, I'll block this PR until the mentioned changes are landed.

I see. But wouldn't it be better to release this change earlier so as not to force everyone to upgrade to the major version with ClickHouse Keeper?

@juan131
Copy link
Contributor

juan131 commented Apr 14, 2025

Hi @ilpianista

wouldn't it be better to release this change earlier so as not to force everyone to upgrade to the major version with ClickHouse Keeper?

I don't think so given your changes don't require a new version since you're only modifying the default values that can be overriden while installing/upgrading the chart.

@ilpianista ilpianista force-pushed the feature/clickhouse-stop-logging branch 2 times, most recently from fddc923 to 61218ca Compare April 14, 2025 13:49
@juan131
Copy link
Contributor

juan131 commented Apr 15, 2025

@ilpianista we just merged the PR replacing ZooKeeper with ClickHouse Keeper bumping the chart major version. Please refer to the upgrading notes to learn more about these changes.

Please note that now the default logging configuration is no longer in the values but in the ConfigMap below:

As it's described in the README this logging configuration can be overriden by mounting a custom configuration file. For instance, using values like the ones below:

configdFiles:
  99-logger.xml: |
    <yandex>
        <logger>
            <level>none</level>
        </logger>
    </yandex>

@ilpianista ilpianista force-pushed the feature/clickhouse-stop-logging branch from 80793c8 to 8030f75 Compare April 15, 2025 09:37
@ilpianista ilpianista force-pushed the feature/clickhouse-stop-logging branch from 8030f75 to 227af1c Compare April 15, 2025 09:38
@ilpianista ilpianista requested a review from juan131 April 15, 2025 09:43
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Thanks so much for rebasing from main and adapting your changes! Please take a look to my suggestions when you have a chance.

ilpianista and others added 3 commits April 15, 2025 13:30
Co-authored-by: Juan Ariza Toledano <[email protected]>
Signed-off-by: Andrea Scarpino <[email protected]>
Co-authored-by: Juan Ariza Toledano <[email protected]>
Signed-off-by: Andrea Scarpino <[email protected]>
Co-authored-by: Juan Ariza Toledano <[email protected]>
Signed-off-by: Andrea Scarpino <[email protected]>
@ilpianista ilpianista requested a review from juan131 April 15, 2025 11:31
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

LGTM

@juan131 juan131 enabled auto-merge (squash) April 15, 2025 11:48
@juan131 juan131 merged commit 24167d1 into bitnami:main Apr 15, 2025
10 of 11 checks passed
@ilpianista ilpianista deleted the feature/clickhouse-stop-logging branch April 15, 2025 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clickhouse solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/clickhouse] db keeps filling up storage even when the database is not in use
5 participants