Skip to content

Feat: Cancel Context#10

Open
krisnadwipayana07 wants to merge 1 commit intoaudipasuatmadi:masterfrom
krisnadwipayana07:krisna/feat/cancel-context
Open

Feat: Cancel Context#10
krisnadwipayana07 wants to merge 1 commit intoaudipasuatmadi:masterfrom
krisnadwipayana07:krisna/feat/cancel-context

Conversation

@krisnadwipayana07
Copy link

@krisnadwipayana07 krisnadwipayana07 commented Mar 25, 2025

Context Cancel test, if the cancel is triggered will not execute batch
even when batch is added, not inserted to microbatch
image

@krisnadwipayana07
Copy link
Author

krisnadwipayana07 commented Mar 25, 2025

Sorry, I forgot to add reviewers @audipasuatmadi
open for discussion, and any feedback are welcome

@audipasuatmadi
Copy link
Owner

Awesome work, thank you for your contribution @krisnadwipayana07.
I suggest having a Unit Test to prove the new behavior. The unit test could be based on the expected behavior rather than implementation details.

@kompiangg, @doug-benn do you have any feedback of this PR?

@doug-benn
Copy link
Collaborator

Yes good work @krisnadwipayana07
Looks fine to me - still not really sure why this would be needed but a feature is a feature
How about a option to force flush on context cancellation? could be helpful if you where ending the process but didn't want to lose data maybe

Also I agree, add a test please and if you want to add some more tests 😉


for {
select {
case <-m.ctx.Done():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this event will be use by users to shutdown the micro batch gracefully and i think there is no other use cases to listen context cancellation if the user does not want to shutdown the microbatch. Regarding to my assumption we need to adjust 2 things at least

  1. Stop method
    Currently if the user want to shutdown the micro batch, user will call Stop method. We must adjust the Stop method to cancel the root context to trigger this event. If not, it will lead to confusion on how to shutdown the microbatch

  2. Flush the leftovers data in the batch
    Regarding to point number 1, we need to flush the leftovers data. We cant just call m.batchCancelCtx() because the routine is already return and batch context is derived from parent context and it will immediately trigger cancel context of all of the child context (see: https://pkg.go.dev/context#WithCancel) and it will need more iteration to listen of cancellation of batch context (since it already return, it will be not possible to listen the event). So, we need to replicate the flow of batch context cancellation in this event

Copy link
Owner

Choose a reason for hiding this comment

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

That's interesting, since we have the Stop method, do we need the context cancellation? what would be the implication?

Open-ended question.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was the point I was trying to make, I don't understand the use case for cancelling it?

Maybe quitting the process gracefully?

I think it would be a good default to flush before the batch ends

Copy link
Collaborator

@kompiangg kompiangg Mar 27, 2025

Choose a reason for hiding this comment

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

That's interesting, since we have the Stop method, do we need the context cancellation? what would be the implication?

Open-ended question.

I think we still need implement Stop method, but the implementation of the method is cancelling the root context. If we not implement Stop method, the flow to gracefully shutdown the microbatch will be implicit, and i think it won't be great experience to use the library

Copy link
Collaborator

@kompiangg kompiangg Mar 27, 2025

Choose a reason for hiding this comment

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

This was the point I was trying to make, I don't understand the use case for cancelling it?

Maybe quitting the process gracefully?

I think it would be a good default to flush before the batch ends

Couldn't agree more, i dont see any cases that need to cancel the root context except for quitting the process gracefully

Exactly, we should flush before the batch ends and i think other library like http and sql will make sure all the process ended before closing the connection gracefully

Copy link
Collaborator

@kompiangg kompiangg left a comment

Choose a reason for hiding this comment

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

I think we need add context cancellation handle on Add method. One of the use case, imagine the user use micro batch in the HTTP Server. The user will likely use Add method in one of the endpoint service. So, it will likely have chance to cancel by http client

@audipasuatmadi
Copy link
Owner

I think we need add context cancellation handle on Add method. One of the use case, imagine the user use micro batch in the HTTP Server. The user will likely use Add method in one of the endpoint service. So, it will likely have chance to cancel by http client

Would the behavior you suggest here means the cancellation is specific to the particular data being added? or the whole buffer?

@kompiangg
Copy link
Collaborator

I think we need add context cancellation handle on Add method. One of the use case, imagine the user use micro batch in the HTTP Server. The user will likely use Add method in one of the endpoint service. So, it will likely have chance to cancel by http client

Would the behavior you suggest here means the cancellation is specific to the particular data being added? or the whole buffer?

Exactly, the cancellation is specific to the particular data being added. It will help us scale-up the Add method to execute PreAdd or PostAdd in the future

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.

4 participants