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.

Sign up for GitHub

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

Jump to bottom

Add new parameters CompactionBlobThreshold(SSD) #2984

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add new parameters CompactionBlobThreshold(SSD) #2984

wants to merge 1 commit into from

Conversation

agalibin
Copy link
Collaborator

@agalibin agalibin commented Feb 6, 2025 •
edited
Loading

It is thresholds for the compaction process in bytes. If data size less threshold it will be written to mixed channel and to merged channel in other case.

@agalibin agalibin added blockstore Add this label to run only cloud/blockstore build and tests on PR large-tests Launch large tests for PR labels Feb 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 0a68bf1.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3814 3814 0 0 0 0

@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from 0a68bf1 to d7981e1 Compare February 7, 2025 16:38
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit d7981e1.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3820 3820 0 0 0 0

@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from d7981e1 to 26dbd0a Compare February 10, 2025 09:02
@github-actions GitHub Actions
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 26dbd0a.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3821 3821 0 0 0 0

@agalibin agalibin changed the title Add new parameters CompactionBlobThreshold and CompactionBlobThreshol… Add new parameters CompactionBlobThreshold(SSD) Feb 10, 2025
@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from 26dbd0a to 628fb9c Compare February 11, 2025 12:38
@github-actions GitHub Actions
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 628fb9c.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3825 3825 0 0 0 0

@agalibin agalibin marked this pull request as ready for review February 11, 2025 16:15
@agalibin agalibin removed the request for review from SvartMetal February 11, 2025 16:17
@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from 628fb9c to 34e358e Compare February 12, 2025 08:43
@agalibin agalibin requested a review from drbasic February 12, 2025 08:43
@github-actions GitHub Actions
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 34e358e.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3825 3825 0 0 0 0

@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from 34e358e to cb931e8 Compare February 12, 2025 10:17
@agalibin agalibin requested a review from drbasic February 12, 2025 11:03
@github-actions GitHub Actions
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit cb931e8.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3825 3825 0 0 0 0

@agalibin
Add new parameters CompactionBlobThreshold and CompactionBlobThreshol…
237b5ee 
…dSSD.

It is thresholds in bytes, if data size less threshold it will be written
 to mixed channel and to merged channel in other case.
@agalibin agalibin force-pushed the users/agalibin/nbs-2875-compaction branch from cb931e8 to 237b5ee Compare February 12, 2025 12:20
@github-actions GitHub Actions
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 237b5ee.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
3825 3825 0 0 0 0

yegorskii
yegorskii reviewed Feb 13, 2025
View reviewed changes
cloud/blockstore/libs/storage/core/config.h
@@ -635,6 +635,9 @@ class TStorageConfig
bool GetAutomaticallyEnableBufferCopyingAfterChecksumMismatch() const;
[[nodiscard]] bool GetNonReplicatedVolumeDirectAcquireEnabled() const;
[[nodiscard]] TDuration GetDestroyVolumeTimeout() const;

ui32 GetCompactionToMergedThreshold() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это же про HDD? Может тогда так явно и написать?

Copy link
Collaborator Author

@agalibin agalibin Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

По аналогии с отстальными барьерами, переназвать несложно. Если здесь переназывать, то и остальные не помешало бы, а то как-то странно получается.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Если трешолд распространяется на все типы дисков то ок, но в данном случае он же только к HDD относится

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

я про такое GetThrottlingEnabled() - GetThrottlingEnabledSSD(), GetWriteBlobThreshold - GetWriteBlobThresholdSSD

yegorskii
yegorskii reviewed Feb 13, 2025
View reviewed changes
cloud/blockstore/libs/storage/core/config.h
@@ -635,6 +635,9 @@ class TStorageConfig
bool GetAutomaticallyEnableBufferCopyingAfterChecksumMismatch() const;
[[nodiscard]] bool GetNonReplicatedVolumeDirectAcquireEnabled() const;
[[nodiscard]] TDuration GetDestroyVolumeTimeout() const;

ui32 GetCompactionToMergedThreshold() const;
ui32 GetCompactionToMergedThresholdSSD() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Я вот не уверен что это вообще нужно. Фича про то что для HDD дисков маленькие блобы должны лежать на SSD. Для SSD они и так всегда на них лежат

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

точка проверки одна ... для SSD будет всегда выставлен в ноль и будет работать по-старому, оно так как-то понятнее

Copy link
Collaborator

Choose a reason for hiding this comment

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

Зачем параметр который всегда выставлен в 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

при компакшне мы параметр в любом случае проверим ... или можно конечно параметр убрать и в proto-helpers сразу возвращать ноль для не HDD, тоже вариант

Copy link
Collaborator

Choose a reason for hiding this comment

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

Я все еще не понимаю, зачем нужны два параметра для фичи которая работает только для HDD

Copy link
Collaborator Author

@agalibin agalibin Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Я выше написал вариант решения.
У нас есть код компакшн, которому все равно какие у нас диски и он на основании параметра пишет либо в мержед либо в миксед ... можно добавить проверку на диск прямо туда, а можно сделать через возврат значения параметра ...
Может завтра захотим на SSD делить компакшн по каналам.

yegorskii
yegorskii reviewed Feb 13, 2025
View reviewed changes
cloud/blockstore/libs/storage/partition/part_actor_compaction.cpp
@@ -664,7 +690,7 @@ void TCompactionActor::AddBlobs(const TActorContext& ctx)
auto request = std::make_unique<TEvPartitionPrivate::TEvAddBlobsRequest>(
RequestInfo->CallContext,
CommitId,
TVector<TAddMixedBlob>(),
std::move(mixedBlobs),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ты же проверил обработку TEvPartitionPrivate::TEvAddBlobsRequest? Раньше на компакшене мы не клали mixed блобы, теперь кладем. Там точно ничего менять не надо?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

обязательно проверил, он умеет так и этим пользуются Write акторы

Copy link
Collaborator

@yegorskii yegorskii Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Write акторы не передают Mode = ADD_COMPACTION_RESULT. Ты уверен что при компакшене будут корректно обновлены счетчики?
https://github.com/ydb-platform/nbs/blob/main/cloud/blockstore/libs/storage/partition/part_actor_addblobs.cpp#L111
Этот код работает только для merged блобов, для mixed ничего делать не надо?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yegorskii yegorskii yegorskii left review comments

@drbasic drbasic drbasic approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
blockstore Add this label to run only cloud/blockstore build and tests on PR large-tests Launch large tests for PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agalibin @drbasic @yegorskii