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

[Filestore] move RestoreEndpoints to Endpoint Manager #2502

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[Filestore] move RestoreEndpoints to Endpoint Manager #2502

wants to merge 8 commits into from

Conversation

antonmyagkov
Copy link
Collaborator

@antonmyagkov antonmyagkov commented Nov 14, 2024 •
edited
Loading

issue: #2553

  1. move RestoreEndpoints to endpoint_manager
  2. rename service.h/cpp to endpoint_manager.h/cpp
  3. remove RequireEndpointsKeyring config parameter
  4. Add unit test for RestoreEndpoints

This refactoring is required to implement #2553

@antonmyagkov antonmyagkov added the filestore Add this label to run only cloud/filestore build and tests on PR label Nov 14, 2024
@antonmyagkov antonmyagkov changed the title filestore: remove endpoint if path is not found [Filestore] remove endpoint if path is not found Nov 14, 2024
@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 86c6868.

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

@antonmyagkov antonmyagkov force-pushed the users/myagkov/filestore-remove-endpoint branch from 86c6868 to 8abb838 Compare November 14, 2024 18:37
@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 8abb838.

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

@antonmyagkov antonmyagkov force-pushed the users/myagkov/filestore-remove-endpoint branch 8 times, most recently from 1e14498 to a375478 Compare November 15, 2024 16:02
@github-actions GitHub Actions
Copy link
Contributor

Note

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

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit a375478.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
1947 1946 0 1 0 0

@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 d09d551.

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

@antonmyagkov antonmyagkov force-pushed the users/myagkov/filestore-remove-endpoint branch from d09d551 to 44c6463 Compare November 22, 2024 13:45
@antonmyagkov antonmyagkov changed the title [Filestore] remove endpoint if path is not found [Filestore] move RestoreEndpoints to Endpoint Manager Nov 22, 2024
@github-actions GitHub Actions
Copy link
Contributor

Note

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

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 79e56f3.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
1945 1944 0 1 0 0

@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 609e271.

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

@antonmyagkov antonmyagkov marked this pull request as ready for review November 26, 2024 12:34
qkrorlqr
qkrorlqr reviewed Nov 26, 2024
View reviewed changes
cloud/filestore/config/vhost.proto Outdated Show resolved Hide resolved
cloud/filestore/libs/client/durable.cpp Outdated Show resolved Hide resolved
cloud/filestore/libs/daemon/vhost/bootstrap.cpp
@@ -386,7 +384,9 @@ void TBootstrapVhost::StartComponents()
FILESTORE_LOG_START_COMPONENT(Server);
FILESTORE_LOG_START_COMPONENT(LocalServiceServer);

RestoreKeyringEndpoints();
if (EndpointManager) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

кажется, EndpointManager всегда должен существовать, разве нет?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ну на практике должен существовать всегда, но в теории наверное можно nullptr получить так как создается EndpointManager не в конструкторе, а в InitComponents()

Я больше ориентировался на другой код в этом классе чтобы было единообразно:

void TBootstrapVhost::Drain()
{
    if (EndpointManager) {
        EndpointManager->Drain();
    }
}

#define FILESTORE_LOG_START_COMPONENT(c)
    if (c) {
        c->Start();
        STORAGE_INFO("Started " << #c);
    } 

FILESTORE_LOG_START_COMPONENT(EndpointManager);

cloud/filestore/libs/endpoint/endpoint_manager.cpp Outdated Show resolved Hide resolved
cloud/filestore/libs/endpoint/endpoint_manager.cpp
requestOrError.GetResult());

if (!request) {
// TODO: report critical error
Copy link
Collaborator

Choose a reason for hiding this comment

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

давай в следующем PR вот эти TODO сделаем, это быстро и просто

cloud/filestore/libs/daemon/vhost/bootstrap.cpp
@@ -386,7 +384,9 @@ void TBootstrapVhost::StartComponents()
FILESTORE_LOG_START_COMPONENT(Server);
FILESTORE_LOG_START_COMPONENT(LocalServiceServer);

RestoreKeyringEndpoints();
if (EndpointManager) {
EndpointManager->RestoreEndpoints();
Copy link
Collaborator

Choose a reason for hiding this comment

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

а почему игнорируем возвращаемое значение? точно не надо подождать на future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ну я в этом плане не менял поведение. Внутри RestoreKeyringEndpoints тоже не ждали future.

В blockstore тоже не ждем, только подписываемся на результат:
https://github.com/ydb-platform/nbs/blob/main/cloud/blockstore/libs/daemon/common/bootstrap.cpp#L902

Мне нужна была future как возвращаемое значение чтобы можно было протестировать restore

cloud/filestore/libs/endpoint/endpoint_manager_ut.cpp Show resolved Hide resolved
@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 216df6b.

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

@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 86aa9c9.

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

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

@qkrorlqr qkrorlqr qkrorlqr approved these changes

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

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

Successfully merging this pull request may close these issues.
2 participants
@antonmyagkov @qkrorlqr