Call the OnFlushCompleted handler unconditionally #5207
Conversation
In the case of a shutdown or other reasons in which a flush may not complete, OnFlushCompleted is not called because the Status is not OK. Instead, pass the status through to the event listener via the FlushJobInfo and allow the listener to deal with any failure as they see fit.
There was a problem hiding this comment.
I'm concerned about the behavior change to call OnFlushCompleted unconditionally, when historically we've only called it on success. I think its the correct behavior, but it might create issues if users have been relying on this handler to track SST files created by a flush. We have the OnTableFileCreated listener for that purpose, but not everyone might have switched over.
@siying Your thoughts? Is it safe to call it unconditionally?
|
@riversand963 knows more history about the flush listeners. @ajkr should know the best but I'm not sure whether he can help:) |
|
I share similar concerns with @anand1976, but I think it's feasible to fix. It sounds like correct behavior and we may need to add a |
|
@riversand963 By next release, do you mean next major release, i.e 7.0? |
|
Yes, I wonder whether we should review it now, but postpone the merge until 7.0, assuming we allow public-facing behavior change only upon major releases. |
|
I agree with postponing the merge until 7.0. I'm not sure how we can track it though. Maybe create a new label "Postpone to next major release" and apply it to this PR? |
|
How about using tag or milestone? |
|
@anand1976 are we still going to merge this? |
@riversand963 Mempurge introduces another complication. Currently, we don't call OnFlushCompleted() if the flush resulted in a mempurge instead. If we call it, I don't know what the status should be. |
#5206
In the case of a shutdown or other reasons in which a flush may
not complete, OnFlushCompleted is not called because the Status is
not OK. Instead, pass the status through to the event listener via the
FlushJobInfo and allow the listener to deal with any failure as they
see fit.