Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: simplify journal and restore streamer cancellation #4549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BorysTheDev
Copy link
Contributor

@BorysTheDev BorysTheDev commented Feb 3, 2025

fixes: #4284
refactor ExecutionState: now we can have 3 independent states RUN, CANCELLED, ERROR

@BorysTheDev BorysTheDev requested a review from adiholden February 3, 2025 13:00
src/server/journal/streamer.cc Outdated Show resolved Hide resolved
src/server/journal/streamer.cc Outdated Show resolved Hide resolved
src/server/journal/streamer.h Show resolved Hide resolved
src/server/journal/streamer.h Show resolved Hide resolved
@BorysTheDev BorysTheDev force-pushed the streamer_refactoring branch 2 times, most recently from 4a3cabc to 3735d47 Compare February 4, 2025 10:39
@kostasrim
Copy link
Contributor

kostasrim commented Feb 4, 2025

@BorysTheDev plz do not force push once a PR has comments. It's impossible to keep track of the logical changes between different commits.

  1. If nobody reviewed -> squash and force push as much as you want
  2. If there are reviews -> squash unpushed local commits but leave the previous ones unchanged

@BorysTheDev BorysTheDev changed the title refactor: simplify journal and restore streamer cancellation refactor: simplify journal and restore streamer cancellation NOT READY FOR REVIEW Feb 4, 2025
@BorysTheDev BorysTheDev removed the request for review from adiholden February 4, 2025 11:25
@BorysTheDev BorysTheDev force-pushed the streamer_refactoring branch 4 times, most recently from c6c3c74 to 5c76061 Compare February 14, 2025 09:23
@BorysTheDev BorysTheDev changed the title refactor: simplify journal and restore streamer cancellation NOT READY FOR REVIEW refactor: simplify journal and restore streamer cancellation Feb 14, 2025

if (last_error_) {
LOG(ERROR) << last_error_.Format();
if (cntx_.IsError()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change of the semantics here.

Before: we sleep either if operation::canceled or if we reported via error ReportError.

Now: we only sleep if there was an error. In other words, we no longer treat cancellation as error which is a mistake.

Cancellation is error and that's why errno exists with: ECANCELED == errc::operation_canceled.

I would like us to keep those semantics for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have cancel_error as well. I don't see any problems if you want create cancel error you call reportCancelError() if you want just cancel the process you cancel the process

const Cancellation* ExecutionState::GetCancellation() const {
return this;
}

void ExecutionState::ReportCancelError() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is still being used (in replication and in other places). The issue here is that after this function is called IsError() is true. This is a bug and the correct values should be false and IsCancelled true.

You do not update the state == Cancelled here but this is also not needed. See my top comment regarding the 3 states you introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you report an error even if it CancelError the state will be error

GenericError ReportErrorInternal(GenericError&& err);

enum class State { RUN, CANCELLED, ERROR };
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need these states. You want to figure out if:

  1. Operation cancelled
  2. Operation cancelled with an error

Cancelled operation is an error. That's why we have ReportCancelcedError which cancels the context with the error ECANCELED. Therefore you don't need any of the 3 states here. All you have to do is:

  bool IsError() const {
    return ec && !IsCancelled();
  }

  bool IsCancelled() const {
    return ec && err_ == ECANCELED;
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

In your state machine:

RUN == (ec_ == std::error_code{})
CANCELLED == (ec_ == ECANCELED)
ERROR == (ec_ && !CANCELLED) 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you use ec_ you need mutex and it's slow. Let's have a call.

@BorysTheDev BorysTheDev requested a review from romange February 14, 2025 11:56
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.

refactor Journal and Restore streamers
2 participants