Skip to content

[Core] Remove actor deletion upon job termination - #31019

Merged
rkooo567 merged 6 commits into
ray-project:masterfrom
rkooo567:remove-job-gc-feature
Mar 23, 2023
Merged

[Core] Remove actor deletion upon job termination#31019
rkooo567 merged 6 commits into
ray-project:masterfrom
rkooo567:remove-job-gc-feature

Conversation

@rkooo567

@rkooo567 rkooo567 commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Signed-off-by: SangBin Cho rkooo567@gmail.com

Why are these changes needed?

There's a feature to remove all actor information 5m after the job is terminated. We will remove this feature for 2 reasons.

  1. This feature never worked
      [this](std::function<void(void)> fn, boost::posix_time::milliseconds delay) {
        boost::asio::deadline_timer timer(main_service_);
        timer.expires_from_now(delay);
        timer.async_wait([fn](const boost::system::error_code &error) {
          if (error != boost::asio::error::operation_aborted) {
            fn();
          } else {
            RAY_LOG(WARNING)
                << "The GCS actor metadata garbage collector timer failed to fire. This "
                   "could old actor metadata not being properly cleaned up. For more "
                   "information, check logs/gcs_server.err and logs/gcs_server.out";
          }
        });
      },

The timer is GC'ed as soon as this method is finished, so this callback was never invoked. That said, this feature never worked anyway.
2. For the actual garbage collection, we already have the logic to clean the dead actor metadata when we have more than 10K dead actors cached. So this feature is redundant.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: SangBin Cho <rkooo567@gmail.com>
@rkooo567 rkooo567 assigned ericl, scv119 and rickyyx and unassigned ericl Dec 12, 2022
@rkooo567

Copy link
Copy Markdown
Contributor Author

Note: This is the only part that the timer was used incorrectly.

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 12, 2022
@rkooo567

Copy link
Copy Markdown
Contributor Author

Fixing build errors.. will ping you guys when it is ready

Signed-off-by: SangBin Cho <rkooo567@gmail.com>
Signed-off-by: SangBin Cho <rkooo567@gmail.com>
@rkooo567 rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 22, 2022
@scv119 scv119 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 3, 2023
@stale

stale Bot commented Feb 4, 2023

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 4, 2023
@rkooo567 rkooo567 removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 24, 2023
@rkooo567 rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 22, 2023
@rkooo567

Copy link
Copy Markdown
Contributor Author

it is ready for the review

Signed-off-by: SangBin Cho <rkooo567@gmail.com>
@rkooo567
rkooo567 requested a review from a team as a code owner March 22, 2023 07:54
Signed-off-by: SangBin Cho <rkooo567@gmail.com>
@ollie-iterators

Copy link
Copy Markdown

The documentation bug could be fixed by merging in the recent changes

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

Nice, great clean-up.

@rkooo567
rkooo567 merged commit c94fdb4 into ray-project:master Mar 23, 2023
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
There's a feature to remove all actor information 5m after the job is terminated. We will remove this feature for 2 reasons.

This feature never worked
      [this](std::function<void(void)> fn, boost::posix_time::milliseconds delay) {
        boost::asio::deadline_timer timer(main_service_);
        timer.expires_from_now(delay);
        timer.async_wait([fn](const boost::system::error_code &error) {
          if (error != boost::asio::error::operation_aborted) {
            fn();
          } else {
            RAY_LOG(WARNING)
                << "The GCS actor metadata garbage collector timer failed to fire. This "
                   "could old actor metadata not being properly cleaned up. For more "
                   "information, check logs/gcs_server.err and logs/gcs_server.out";
          }
        });
      },
The timer is GC'ed as soon as this method is finished, so this callback was never invoked. That said, this feature never worked anyway.
2. For the actual garbage collection, we already have the logic to clean the dead actor metadata when we have more than 10K dead actors cached. So this feature is redundant.

Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
There's a feature to remove all actor information 5m after the job is terminated. We will remove this feature for 2 reasons.

This feature never worked
      [this](std::function<void(void)> fn, boost::posix_time::milliseconds delay) {
        boost::asio::deadline_timer timer(main_service_);
        timer.expires_from_now(delay);
        timer.async_wait([fn](const boost::system::error_code &error) {
          if (error != boost::asio::error::operation_aborted) {
            fn();
          } else {
            RAY_LOG(WARNING)
                << "The GCS actor metadata garbage collector timer failed to fire. This "
                   "could old actor metadata not being properly cleaned up. For more "
                   "information, check logs/gcs_server.err and logs/gcs_server.out";
          }
        });
      },
The timer is GC'ed as soon as this method is finished, so this callback was never invoked. That said, this feature never worked anyway.
2. For the actual garbage collection, we already have the logic to clean the dead actor metadata when we have more than 10K dead actors cached. So this feature is redundant.

Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants