Skip to content

Delete cookie as part of decommission API - #4592

Merged
zymap merged 2 commits into
apache:masterfrom
SongOf:delete_cookie_in_decommission_api
Dec 5, 2025
Merged

Delete cookie as part of decommission API#4592
zymap merged 2 commits into
apache:masterfrom
SongOf:delete_cookie_in_decommission_api

Conversation

@SongOf

@SongOf SongOf commented Apr 20, 2025

Copy link
Copy Markdown
Contributor

Implement the logic in BP-68: Delete cookie as part of decommission API.

@StevenLuMT

Copy link
Copy Markdown
Member

I think it's the same as this historical PR #4541 . I think it's quite high risk and there's no need to add this function.
@SongOf

@StevenLuMT StevenLuMT closed this May 18, 2025
@StevenLuMT

Copy link
Copy Markdown
Member

I suggest temporarily closing this PR and reopening it later if necessary.

@StevenLuMT

Copy link
Copy Markdown
Member

The command line decommission will delete cookies by default, but the http api does not have the alignment function

@StevenLuMT StevenLuMT reopened this May 19, 2025

@StevenLuMT StevenLuMT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good jobs:
The command line decommission will delete cookies by default, but the http api does not have the alignment function

@StevenLuMT

Copy link
Copy Markdown
Member

@hezhangjian @zymap help review this pr, thanks

Comment on lines +781 to +792
Thread.sleep(60 * 1000);
runFunctionWithRegistrationManager(baseConf, registrationManager -> {
Versioned<Cookie> cookieFromZk = null;
try {
cookieFromZk = Cookie.readFromRegistrationManager(registrationManager,
bookieId);
} catch (BookieException e) {
} finally {
assertTrue(cookieFromZk != null);
}
return true;
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use Awaitility to verify the condition periodically instead of sleeping.

        Awaitility.await().untilAsserted(() -> {
            assertTrue("Bookie should be running and converted to readonly mode",
                    bookie.isRunning() && bookie.isReadOnly());
        });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can use Awaitility to verify the condition periodically instead of sleeping.

        Awaitility.await().untilAsserted(() -> {
            assertTrue("Bookie should be running and converted to readonly mode",
                    bookie.isRunning() && bookie.isReadOnly());
        });

fixed, Thanks for your suggestion.

HttpServiceResponse response1 = decommissionService.handle(request1);
assertEquals(HttpServer.StatusCode.OK.getValue(), response1.getStatusCode());
// wait decommission finish
Thread.sleep(60 * 1000);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same above.

fixed, Thanks for your suggestion.

@SongOf
SongOf force-pushed the delete_cookie_in_decommission_api branch from 4798852 to eb8be0a Compare September 6, 2025 15:03
@zymap zymap added this to the 4.18.0 milestone Nov 26, 2025
@zymap zymap closed this Nov 26, 2025
@zymap zymap reopened this Nov 26, 2025
@zymap
zymap merged commit 341f7d7 into apache:master Dec 5, 2025
40 of 42 checks passed

@AbdulElahOthmanGwaith AbdulElahOthmanGwaith left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

// wait decommission finish
Awaitility.await()
.untilAsserted(() -> {
Versioned clusterInfo =
admin.getClusterInfo();
assertFalse(
clusterInfo.getValue()
.getBookies()
.contains(bookieId)
);
});

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants