Skip to content

Conversation

@SongOf
Copy link
Contributor

@SongOf SongOf commented Apr 20, 2025

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

@StevenLuMT
Copy link
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
Member

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

@StevenLuMT
Copy link
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
Copy link
Member

@StevenLuMT StevenLuMT left a comment

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
Member

@hezhangjian @zymap help review this pr, thanks

Comment on lines 781 to 795
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
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
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
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
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
Copy link

@AbdulElahOthmanGwaith AbdulElahOthmanGwaith left a comment

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