[native] Implement Graceful Shutdown in Native worker #23517

anandamideShakyan · 2024-08-25T12:19:05Z

Description

Added PUT method in /v1/info/state for transitioning the server to the SHUTTING_DOWN state for graceful shutdown

Motivation and Context

The Prestissimo project currently lacks support for transitioning the server to the SHUTTING_DOWN state via the PUT method in the /v1/info/state endpoint. Implementing this feature allows for a graceful shutdown of the server, ensuring that ongoing processes are completed without abrupt termination.

Impact

This implementation introduces a new feature that enables the server to handle a graceful shutdown when the SHUTTING_DOWN state is triggered via a PUT request. By supporting this functionality, the server can now properly manage ongoing requests before shutting down, reducing the risk of data loss or inconsistency.

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

linux-foundation-easycla · 2024-08-25T12:19:09Z

The committers listed above are authorized under a signed CLA.

✅ login: anandamideShakyan / name: Shakyan Kushwaha (2879ca5)

aditi-pandit

Thanks for this code. @anandamideShakyan : Please can you add a unit test (maybe in ServerOperationsTest or a new unit test file) for this functionality.

anandamideShakyan · 2024-08-26T09:21:31Z

@aditi-pandit Sure, I will add the unit test.

amitkdutta

@anandamideShakyan Prestoserver handles graceful shutdown mechanism with SIGTERM, which is a standard way to shut down any process. Here is how the signal handler is registered (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoServer.cpp#L152)

Also in the stop method], server goes in graceful shotdown mode

As a result, no explicit endpoint is required to ask the server to shut down. Just sending SIGTERM like any other process works as expected. In Java, perhaps these signal handling is not possible, hence this end point concept came into development. Native worker even have additional signal handling mechnism to catch debugging information in case of SEGFault (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoMain.cpp#L25C27-L25C47)

CC: @spershin

czentgr

Thank you for adding this.

presto-native-execution/presto_cpp/main/PrestoServer.cpp

anandamideShakyan · 2024-08-28T17:41:05Z

@anandamideShakyan Prestoserver handles graceful shutdown mechanism with SIGTERM, which is a standard way to shut down any process. Here is how the signal handler is registered (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoServer.cpp#L152)

Also in the stop method], server goes in graceful shotdown mode

As a result, no explicit endpoint is required to ask the server to shut down. Just sending SIGTERM like any other process works as expected. In Java, perhaps these signal handling is not possible, hence this end point concept came into development. Native worker even have additional signal handling mechnism to catch debugging information in case of SEGFault (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoMain.cpp#L25C27-L25C47)

CC: @spershin

@amitkdutta

Please refer to this issue : #23299

amitkdutta · 2024-09-05T16:52:41Z

@anandamideShakyan Prestoserver handles graceful shutdown mechanism with SIGTERM, which is a standard way to shut down any process. Here is how the signal handler is registered (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoServer.cpp#L152)
Also in the stop method], server goes in graceful shotdown mode
As a result, no explicit endpoint is required to ask the server to shut down. Just sending SIGTERM like any other process works as expected. In Java, perhaps these signal handling is not possible, hence this end point concept came into development. Native worker even have additional signal handling mechnism to catch debugging information in case of SEGFault (https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/PrestoMain.cpp#L25C27-L25C47)
CC: @spershin

@amitkdutta

Please refer to this issue : #23299

@anandamideShakyan Commented in the issue as well

#23299 (comment)

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/PrestoServer.h

presto-native-execution/presto_cpp/main/PrestoServer.cpp

czentgr · 2024-10-07T17:04:56Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -1404,6 +1413,28 @@ void PrestoServer::reportNodeStatus(proxygen::ResponseHandler* downstream) {
  http::sendOkResponse(downstream, json(fetchNodeStatus()));
 }

+void PrestoServer::handleGracefulShutdown(const std::vector<std::unique_ptr<folly::IOBuf>>& body, proxygen::ResponseHandler* downstream){


The format check is failing. Please fix this (see the Makefile having a format-fix target).

czentgr · 2024-10-07T17:09:44Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -1404,6 +1413,28 @@ void PrestoServer::reportNodeStatus(proxygen::ResponseHandler* downstream) {
  http::sendOkResponse(downstream, json(fetchNodeStatus()));
 }

+void PrestoServer::handleGracefulShutdown(const std::vector<std::unique_ptr<folly::IOBuf>>& body, proxygen::ResponseHandler* downstream){
+    if (body.size()==1 && body[0]->moveToFbString() == "\"SHUTTING_DOWN\"") {
+         // Print message and initiate shutdown


We probably don't need all these comments as they explains what happens which is apparent from the code. We use the Velox comment guidelines: https://github.com/facebookincubator/velox/blob/main/CONTRIBUTING.md#coding-best-practices (see "Code Comments" section).

majetideepak · 2024-10-10T10:37:29Z

Please add a unit test as Aditi requested above.

anandamideShakyan · 2024-10-11T12:30:58Z

@majetideepak I have added the unit test GracefulShutdownTest.cpp.

majetideepak

@anandamideShakyan can you check if can use PrestoServerOperations.h?
See the test presto_cpp/main/tests/ServerOperationTest.cpp.
I see that Aditi referenced this as well.

anandamideShakyan · 2024-10-25T12:24:34Z

@majetideepak I had to add GracefulShutdownTest.h as a separate file because if we are running the presto server after some test cases we get error The memory manager has already been set, the PrestoServer initializes global memory manager and there are checks in the initializer to see if an instance is already created.
Also the reason for adding is as a separate executable is that when the presto server exits, the whole process would exit and I used to get SIGABRT.

majetideepak

after some test cases we get error The memory manager has already been set

This should not happen. Can you share a reproducer?
Let's extend class PrestoServerOperations if you need to launch presto server in a separate thread.

majetideepak · 2024-10-25T17:14:13Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

-  // Give coordinator some time to receive our new node state and stop sending
-  // any tasks.
-  std::this_thread::sleep_for(std::chrono::seconds(shutdownOnsetSec));
+  std::thread shutdownThread([this, shutdownOnsetSec]() {


what is the motivation for a separate shutdown thread?

We were not able to read the server status if we didn't schedule the shutdown in a separate thread because before we could read the status, server was shutting down.

Why do we need to read this status?

We need it to compare it against the status codes in the test cases.

Again not ideal to make changes only for testing.

majetideepak · 2024-10-25T17:18:09Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -157,7 +158,9 @@ PrestoServer::PrestoServer(const std::string& configDirectoryPath)

 PrestoServer::~PrestoServer() {}

-void PrestoServer::run() {
+void PrestoServer::run(
+    std::function<void(proxygen::HTTPServer*)> onSuccess,


How are these arguments used?

We use it inside PrestoServerWrapper.cpp to get the socket address of the server that we need while running the test suite(see SetUpTestSuite()).

It is not ideal to make API changes just for testing. Let's figure out another way to test.

anandamideShakyan · 2024-10-27T12:25:42Z

This should not happen. Can you share a reproducer? Let's extend class PrestoServerOperations if you need to launch presto server in a separate thread.

@majetideepak
I extended the ServerOperationTest.cpp to include the graceful shutdown test suite,I get this :
[----------] 7 tests from ServerOperationTest I20241027 17:00:11.820835 20054 Configs.cpp:120] [PRESTO_STARTUP] Registered properties from '../../../etc/config.properties': runtime-metrics-collection-enabled=true register-test-functions=true http-server.http.port=7777 shutdown-onset-sec=1 presto.version=testversion discovery.uri=http://127.0.0.1:49595 I20241027 17:00:11.821100 20054 Configs.cpp:120] [PRESTO_STARTUP] Registered properties from '../../../etc/node.properties': node.internal-address=127.0.0.1 node.location=testing-location node.environment=testing I20241027 17:00:11.821332 20054 Configs.cpp:120] [PRESTO_STARTUP] Registered properties from '../../../etc/velox.properties': mutable-config=true I20241027 17:00:11.822000 20054 Configs.cpp:900] [PRESTO_STARTUP] Updated in '../../../etc/velox.properties' from SystemProperties: max_partial_aggregation_memory=16777216 max_page_partitioning_buffer_size=33554432 max_output_buffer_size=33554432 presto.array_agg.ignore_nulls=false I20241027 17:00:11.822346 20054 PrestoServer.cpp:799] [PRESTO_STARTUP] Starting with node memory 40GB E20241027 17:00:11.822404 20054 Exceptions.h:66] Line: /Users/shakyan/opensource/presto/presto-native-execution/velox/velox/common/memory/Memory.cpp:177, Function:initialize, Expression: instance == nullptr The memory manager has already been set: Memory Manager[capacity UNLIMITED alignment 64B usedBytes 0B number of pools 2 List of root pools: __sys_root__ Memory Allocator[MALLOC capacity UNLIMITED allocated bytes 0 allocated pages 0 mapped pages 0] ARBIRTATOR[NOOP CAPACITY[UNLIMITED]]], Source: RUNTIME, ErrorCode: INVALID_STATE libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxRuntimeError: Exception: VeloxRuntimeError Error Source: RUNTIME Error Code: INVALID_STATE Reason: The memory manager has already been set: Memory Manager[capacity UNLIMITED alignment 64B usedBytes 0B number of pools 2 List of root pools: __sys_root__ Memory Allocator[MALLOC capacity UNLIMITED allocated bytes 0 allocated pages 0 mapped pages 0] ARBIRTATOR[NOOP CAPACITY[UNLIMITED]]] Retriable: False Expression: instance == nullptr Function: initialize File: /Users/shakyan/opensource/presto/presto-native-execution/velox/velox/common/memory/Memory.cpp Line: 177 Process finished with exit code 134 (interrupted by signal 6:SIGABRT)
Steps to reproduce:
Add the following to ServerOperationTest.cpp

#include "presto_cpp/main/http/tests/HttpTestBase.h"
#include "presto_cpp/main/tests/PrestoServerWrapper.h"
.......
namespace facebook::presto {
static std::unique_ptr<facebook::presto::PrestoServer> getPrestoServer() {
    auto server = std::make_unique<PrestoServer>("../../../etc");
    return server;
}
........
class ServerOperationTest : public testing::Test {
protected:
    static facebook::presto::test::PrestoServerWrapper* wrapper;
    static folly::SocketAddress* socketAddress;
    static void SetUpTestSuite() {
        folly::SingletonVault::singleton()->registrationComplete();
#ifndef PRESTO_STATS_REPORTER_TYPE
        // Initialize singleton for the reporter.
        folly::Singleton<facebook::velox::BaseStatsReporter> reporter(
            []() { return new facebook::velox::DummyStatsReporter(); });
#endif
        static auto prestoServer = getPrestoServer();
        wrapper = new facebook::presto::test::PrestoServerWrapper(
            std::move(prestoServer));
        socketAddress = new folly::SocketAddress(wrapper->start().get());
    }

    static void TearDownTestSuite() {
        wrapper->stop();
    }
.........
};
facebook::presto::test::PrestoServerWrapper* ServerOperationTest::wrapper =
    nullptr;
folly::SocketAddress* ServerOperationTest::socketAddress = nullptr;
..........
TEST_F(ServerOperationTest, TestGetState) {
    auto memoryPool = memory::MemoryManager::getInstance()->addLeafPool("");
    HttpClientFactory clientFactory;
    auto client = clientFactory.newClient(
        *socketAddress,
        std::chrono::milliseconds(1'000),
        std::chrono::milliseconds(0),
        false,
        memoryPool);
    {
        auto response = sendGet(client.get(), "/v1/info/state").get();
        ASSERT_EQ(response->headers()->getStatusCode(), http::kHttpOk);
    }
}
TEST_F(ServerOperationTest, TestSendPutShuttingDown) {
    auto memoryPool = memory::MemoryManager::getInstance()->addLeafPool("");
    HttpClientFactory clientFactory;
    auto client = clientFactory.newClient(
        *socketAddress,
        std::chrono::milliseconds(1'000),
        std::chrono::milliseconds(0),
        false,
        memoryPool);
    {
        std::string emptyBody = "";
        auto response = sendPut(client.get(), "/v1/info/state", 0, emptyBody).get();
        ASSERT_EQ(
            response->headers()->getStatusCode(),
            http::kHttpBadRequest); // Assuming empty request is bad
        ASSERT_EQ(bodyAsString(*response, memoryPool.get()), "Bad Request");
    }
    {
        std::string invalidBody = "\"SHUTTING_DWN\"";
        auto response =
            sendPut(client.get(), "/v1/info/state", 0, invalidBody).get();
        ASSERT_EQ(response->headers()->getStatusCode(), http::kHttpBadRequest);
        ASSERT_EQ(bodyAsString(*response, memoryPool.get()), "Bad Request");
    }
    {
        std::string body = "\"SHUTTING_DOWN\"";
        auto response = sendPut(client.get(), "/v1/info/state", 0, body).get();
        ASSERT_EQ(response->headers()->getStatusCode(), http::kHttpOk);
    }
}
} // namespace facebook::presto

While working on the issue I discussed the errors that I was getting with @nmahadevuni, he suggested me to make gracefulshutdown as a separate file and a separate executable. This solved the issue. Aditi also said that I can use a new file.

anandamideShakyan · 2024-10-28T07:24:27Z

@majetideepak , I can add the test suite as a mock test. But it still won't test the whole client->RESTEndpoint->response.
I can shift testcase to ServerOperationsTest.cpp and use serverOperationSetState with actual server created. But we would still have that testcase as a separate executable. Also it doesn't test the REST endpoint, it will call the shutdown method directly. Do you want me to go ahead with this approach?

majetideepak · 2024-10-28T11:40:22Z

test the whole client->RESTEndpoint->response

@anandamideShakyan We already have the setup for such e2e in the Java tests. Can we add a test there?

…e SHUTING_DOWN state for graceful shutdown

majetideepak

@anandamideShakyan This looks great! I have 2 minor comments.

majetideepak · 2024-11-06T20:42:27Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

+      folly::trimWhitespace(body[0]->moveToFbString()) == "\"SHUTTING_DOWN\"") {
+    LOG(INFO) << "Shutdown requested";
+    if (nodeState() == NodeState::kActive) {
+      std::thread([this]() { this->stop(); }).detach();


Why do we invoke stop in a separate thread? Please add a comment.

majetideepak · 2024-11-06T20:43:43Z

...ecution/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeGracefulShutdown.java

+        return PrestoNativeQueryRunnerUtils.createNativeQueryRunner(true);
+    }
+
+    private String getServerUri(QueryRunner queryRunner)


nit: do we need a function if it is used only once?

anandamideShakyan requested a review from a team as a code owner August 25, 2024 12:19

aditi-pandit reviewed Aug 26, 2024

View reviewed changes

anandamideShakyan changed the title ~~[native] Added PUT method in /v1/info/state for Graceful Shutdown~~ [native] Implement Graceful Shutdown in Native worker Aug 26, 2024

amitkdutta requested changes Aug 26, 2024

View reviewed changes

czentgr reviewed Aug 26, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

majetideepak reviewed Sep 19, 2024

View reviewed changes

anandamideShakyan force-pushed the develop branch from ff8a9f2 to f787527 Compare October 4, 2024 20:05

majetideepak reviewed Oct 5, 2024

View reviewed changes

majetideepak reviewed Oct 7, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated Show resolved Hide resolved

anandamideShakyan force-pushed the develop branch 2 times, most recently from a6bbcb4 to c1d9c3d Compare October 7, 2024 13:59

czentgr reviewed Oct 7, 2024

View reviewed changes

anandamideShakyan force-pushed the develop branch from c1d9c3d to 1f9ab83 Compare October 10, 2024 07:31

anandamideShakyan force-pushed the develop branch 2 times, most recently from 9502bfe to 6325ff8 Compare October 11, 2024 12:05

anandamideShakyan force-pushed the develop branch 3 times, most recently from b9a39ef to 4186e82 Compare October 11, 2024 16:12

prestodb-ci linked an issue Oct 18, 2024 that may be closed by this pull request

[Native] Support Graceful Shutdown in Prestissimo #23299

Open

majetideepak reviewed Oct 25, 2024

View reviewed changes

anandamideShakyan force-pushed the develop branch 2 times, most recently from 1acfb72 to 32f1458 Compare November 6, 2024 15:28

Added PUT method in /v1/info/state for transitioning the server to th…

2879ca5

…e SHUTING_DOWN state for graceful shutdown

anandamideShakyan force-pushed the develop branch from 70e697a to 2879ca5 Compare November 6, 2024 19:02

majetideepak reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Implement Graceful Shutdown in Native worker #23517

[native] Implement Graceful Shutdown in Native worker #23517

anandamideShakyan commented Aug 25, 2024

linux-foundation-easycla bot commented Aug 25, 2024 •

edited

Loading

aditi-pandit left a comment •

edited

Loading

anandamideShakyan commented Aug 26, 2024

amitkdutta left a comment •

edited

Loading

czentgr left a comment

anandamideShakyan commented Aug 28, 2024

amitkdutta commented Sep 5, 2024

czentgr Oct 7, 2024

anandamideShakyan Oct 11, 2024

czentgr Oct 7, 2024

majetideepak Oct 7, 2024

majetideepak commented Oct 10, 2024

anandamideShakyan commented Oct 11, 2024

majetideepak left a comment •

edited

Loading

anandamideShakyan commented Oct 25, 2024

majetideepak left a comment

majetideepak Oct 25, 2024

anandamideShakyan Oct 28, 2024

majetideepak Oct 28, 2024

anandamideShakyan Oct 28, 2024

majetideepak Oct 28, 2024 •

edited

Loading

majetideepak Oct 25, 2024

anandamideShakyan Oct 27, 2024

majetideepak Oct 28, 2024

anandamideShakyan commented Oct 27, 2024 •

edited by majetideepak

Loading

anandamideShakyan commented Oct 28, 2024

majetideepak commented Oct 28, 2024

majetideepak left a comment

majetideepak Nov 6, 2024

majetideepak Nov 6, 2024

[native] Implement Graceful Shutdown in Native worker #23517

Are you sure you want to change the base?

[native] Implement Graceful Shutdown in Native worker #23517

Conversation

anandamideShakyan commented Aug 25, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Aug 25, 2024 • edited Loading

aditi-pandit left a comment • edited Loading

Choose a reason for hiding this comment

anandamideShakyan commented Aug 26, 2024

amitkdutta left a comment • edited Loading

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

anandamideShakyan commented Aug 28, 2024

amitkdutta commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Oct 10, 2024

anandamideShakyan commented Oct 11, 2024

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

anandamideShakyan commented Oct 25, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandamideShakyan commented Oct 27, 2024 • edited by majetideepak Loading

anandamideShakyan commented Oct 28, 2024

majetideepak commented Oct 28, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Aug 25, 2024 •

edited

Loading

aditi-pandit left a comment •

edited

Loading

amitkdutta left a comment •

edited

Loading

majetideepak left a comment •

edited

Loading

majetideepak Oct 28, 2024 •

edited

Loading

anandamideShakyan commented Oct 27, 2024 •

edited by majetideepak

Loading