Improve service shutdown #381

mdemoret-nv · 2023-09-08T00:47:11Z

Description

This PR incorporates several changes from the MNMG work into the current code base. The main goal of this PR was to eliminate the unnecessary warnings generated during tests. Such as:

E20230905 22:54:41.207543  6472 service.cpp:136] mrc::service: service was not joined before being destructed; issuing join

This PR includes the following improvements (all taken from MNMG):

Improved lifetime handling of Service classes
Required name for all Service classes to help with debugging
Improved thread naming to fit within the 15 character limit
Added service tests file for ensuring exceptions and state is properly handled

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

cwharris · 2023-09-08T20:54:10Z

cpp/mrc/src/internal/system/fiber_task_queue.cpp

@@ -106,7 +110,7 @@ void FiberTaskQueue::launch(task_pkg_t&& pkg) const
    boost::fibers::fiber fiber(std::move(pkg.first));
    auto& props(fiber.properties<FiberPriorityProps>());
    props.set_priority(pkg.second.priority);
-    DVLOG(10) << *this << ": created fiber " << fiber.get_id() << " with priority " << pkg.second.priority;
+    DVLOG(20) << *this << ": created fiber " << fiber.get_id() << " with priority " << pkg.second.priority;


Question: What is 10 and 20 here? Is there a name we could use that would provide more meaningful information?

No consistent meaning, but there is an issue for that already here: #76

cpp/mrc/src/internal/data_plane/data_plane_resources.cpp

cpp/mrc/src/internal/executor/executor_definition.cpp

cpp/mrc/src/internal/control_plane/client.cpp

cpp/mrc/src/internal/service.hpp

cwharris · 2023-09-08T22:27:34Z

cpp/mrc/src/internal/control_plane/client.hpp


    template <typename MessageT>
-    void issue_event(const protos::EventType& event_type, MessageT&& message);
+    AsyncEventStatus issue_event(const protos::EventType& event_type, MessageT&& message);


Suggestion: Rename to issue_event_async if it is an async function as indicated by the return type.

cwharris · 2023-09-08T22:27:40Z

cpp/mrc/src/internal/control_plane/client.hpp


-    void issue_event(const protos::EventType& event_type);
+    AsyncEventStatus issue_event(const protos::EventType& event_type);


Suggestion: Rename to issue_event_async if it is an async function as indicated by the return type.

cpp/mrc/src/internal/control_plane/client.hpp

cwharris · 2023-09-08T22:32:25Z

cpp/mrc/src/internal/control_plane/client.hpp

@@ -150,6 +198,8 @@ class Client final : public resources::PartitionResourceBase, public Service
    void request_update();

  private:
+    AsyncEventStatus write_event(protos::Event event, bool await_response = false);


Suggestion: Rename to write_event_async if it is an async function as indicated by the return type.

cwharris · 2023-09-08T22:38:16Z

cpp/mrc/src/tests/test_service.cpp

+    ~SimpleService() override
+    {
+        if (m_do_call_in_destructor)
+        {
+            Service::call_in_destructor();
+        }
+    }


Question: If m_do_call_in_destructor and call_in_destructor are both declared in Service, why not have Service's destructor container this logic?

call_in_destructor can call the virtual methods for stopping and killing. So you cant execute virtual functions in a base class destructor.

codecov · 2023-09-12T02:09:08Z

Codecov Report

Merging #381 (913cc12) into branch-23.11 (9b47924) will increase coverage by 0.43%.
Report is 7 commits behind head on branch-23.11.
The diff coverage is 85.87%.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.11     #381      +/-   ##
================================================
+ Coverage         73.11%   73.54%   +0.43%     
================================================
  Files               382      385       +3     
  Lines             13403    13640     +237     
  Branches           1010     1025      +15     
================================================
+ Hits               9800    10032     +232     
- Misses             3603     3608       +5

Flag	Coverage Δ
cpp	`69.69% <86.83%> (+0.63%)`	⬆️
py	`41.95% <32.63%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...mrc/src/internal/control_plane/client/instance.cpp	`37.20% <ø> (ø)`
...rnal/control_plane/client/subscription_service.cpp	`0.00% <ø> (ø)`
...c/src/internal/data_plane/data_plane_resources.cpp	`72.50% <ø> (ø)`
cpp/mrc/src/internal/data_plane/server.cpp	`55.65% <ø> (ø)`
.../mrc/src/internal/executor/executor_definition.cpp	`78.43% <ø> (-0.42%)`	⬇️
cpp/mrc/src/internal/pipeline/manager.cpp	`86.95% <ø> (ø)`
cpp/mrc/src/internal/remote_descriptor/manager.cpp	`63.06% <ø> (ø)`
cpp/mrc/src/internal/service.hpp	`0.00% <0.00%> (ø)`
cpp/mrc/src/tests/test_pipeline.cpp	`97.58% <ø> (-0.02%)`	⬇️
cpp/mrc/include/mrc/core/utils.hpp	`45.16% <50.00%> (+1.16%)`	⬆️
... and 20 more

... and 7 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa5e40c...913cc12. Read the comment docs.

@mdemoret-nv

* Based on fixes from @mdemoret-nv : https://github.com/mdemoret-nv/MRC/tree/mdd_control-plane-promises & #381 * Adopts updated versions of boost, clang & IWYU * Only run IWYU on files changed in PR * Use clang for checks stage * Allocate progress engine promises on the heap fixes #379 Authors: - David Gardner (https://github.com/dagardner-nv) - Michael Demoret (https://github.com/mdemoret-nv) Approvers: - Michael Demoret (https://github.com/mdemoret-nv) - Christopher Harris (https://github.com/cwharris) URL: #391

mdemoret-nv · 2023-09-25T16:08:03Z

Closing since these changes are included in #391 which has been merged.

mdemoret-nv added 4 commits September 7, 2023 13:03

Updating control plane client to maintain promise lifetimes

1fad5eb

Removing Service constructor with description

44da13f

Incorporating updates from MNMG that make services more stable

74f988b

Getting tests to pass

34b4166

mdemoret-nv added bug Something isn't working non-breaking Non-breaking change labels Sep 8, 2023

mdemoret-nv requested a review from a team as a code owner September 8, 2023 00:47

cwharris requested changes Sep 8, 2023

View reviewed changes

Crash is no longer happening

913cc12

This was referenced Sep 18, 2023

Safe handling of control plane promises & fix CI #389

Closed

Safe handling of control plane promises & fix CI #391

Merged

jarmak-nv assigned mdemoret-nv Sep 20, 2023

mdemoret-nv closed this Sep 25, 2023

mdemoret-nv deleted the mdd_fix-service-shutdown branch September 25, 2023 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve service shutdown #381

Improve service shutdown #381

mdemoret-nv commented Sep 8, 2023 •

edited

Loading

cwharris Sep 8, 2023

mdemoret-nv Sep 21, 2023

cwharris Sep 8, 2023

cwharris Sep 8, 2023

cwharris Sep 8, 2023

cwharris Sep 8, 2023

mdemoret-nv Sep 21, 2023

codecov bot commented Sep 12, 2023

mdemoret-nv commented Sep 25, 2023


		void issue_event(const protos::EventType& event_type);
		AsyncEventStatus issue_event(const protos::EventType& event_type);

Improve service shutdown #381

Improve service shutdown #381

Conversation

mdemoret-nv commented Sep 8, 2023 • edited Loading

Description

Checklist

cwharris Sep 8, 2023

Choose a reason for hiding this comment

mdemoret-nv Sep 21, 2023

Choose a reason for hiding this comment

cwharris Sep 8, 2023

Choose a reason for hiding this comment

cwharris Sep 8, 2023

Choose a reason for hiding this comment

cwharris Sep 8, 2023

Choose a reason for hiding this comment

cwharris Sep 8, 2023

Choose a reason for hiding this comment

mdemoret-nv Sep 21, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2023

Codecov Report

mdemoret-nv commented Sep 25, 2023

mdemoret-nv commented Sep 8, 2023 •

edited

Loading