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

Missing feature to terminate processing #718

Open
Zordid opened this issue Feb 11, 2024 · 6 comments
Open

Missing feature to terminate processing #718

Zordid opened this issue Feb 11, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@Zordid
Copy link

Zordid commented Feb 11, 2024

I just evaluated this library for use in our product. But sadly, the missing termination feature from within the poll lambda is a show stopper. How come such a trivial and highly useful feature is missing? If I can only fail a record over and over again, there must be a way to stop processing gracefully altogether.
Is this feature coming soon?

And why is the poll lambda relying on exception handling instead of a proper return value? Using exception handling for flow control is very outdated and should not be the norm anymore.

Thanks!

@rkolesnev
Copy link
Member

Hi @Zordid - can you please elaborate? I do not fully follow - what do you mean by termination feature?

Afaik you can close parallel consumer instance from within poll call - either signal main app thread that it should close the Parallel Consumer instance or just call the close from within poll directly on a different thread - something like this:

        parallelConsumer.poll(recordContexts -> {
            if (shouldSignalClose()) {
                CompletableFuture.runAsync(() -> parallelConsumer.closeDontDrainFirst(Duration.ofSeconds(10)));
            }
            processRecord(recordContexts);
        });

Normally though application would not close Parallel Consumer due to failing record - if the issue that leads to record processing to fail - is bad data in the record - then Dead Letter Queue approach could be used or that record just skipped and processing moved to next record - depending on requirements, but the general goal is to not block processing.

If the processing is failing due to external system being unavailable - then that could be monitored through health monitoring of dependent systems and ParallelConsumer can be paused / closed on unavailability of external system (similar to circuit breaker pattern) and resumed / restarted when health of external system is back to good / green - that, in my opinion, would be triggered outside of the Poll lambda though.

Using exceptions to indicate a failure is normal enough in applications to my knowledge - could you elaborate ?

@Zordid
Copy link
Author

Zordid commented Feb 15, 2024

All I mean is, it is a cleaner approach to give the lambda a way to indicate process result by a return value than to use exceptions. General rule of thumb for me, because exceptions ar least make it very hard to understand the contract by looking at the signature of a function.

If the lambda that processes the record would need to return a plain enum with ok, retry or stop it would be much easier to understand.

Well, the reason stop is a thing we need is quite simple. The product we build does not know anything about its context, it is all driven by configuration of a customer.

And one of the possible reactions for errors is to shut down the application - the host environment would then restart it all and there is a chance the problem is solved.

If it were just a pure Kafka app, I'd agree, but we run completely different things connecting to the very unpleasant SAP world in this app...

Cheers and thanks for one trick to kill the processing from inside!

@rkolesnev rkolesnev added the enhancement New feature or request label Feb 22, 2024
@krvajal
Copy link

krvajal commented Mar 1, 2024

How come such a trivial and highly useful feature is missing?

This is not the right attitude, specially for free software

@Zordid
Copy link
Author

Zordid commented Mar 2, 2024

Using exceptions to indicate a failure is normal enough in applications to my knowledge - could you elaborate ?

I think I forgot to elaborate on this. IMHO exception have had their time. I am coming from a functional standpoint and love interfaces that are clear&sleek and use a functional approach rather than relying on (hidden) contracts of how exceptions are meant to be thrown and caught elsewhere. Exceptions are the modern goto, after all. Whenever possible, an API should try to be free of such inner behaviour and relying on user code to throw exceptions to trigger different paths of execution.

@rkolesnev
Copy link
Member

Personally, in general - i have a view that "it depends" - how application / library is written, what expectations are of its users etc.
I find that Java functional interfaces are a bit lacking when it comes to handling branching logic - pattern matching, exception flows etc - i've seen and used that more in Scala, or using something like Vert.x with future handler composition.

Specifically with Parallel Consumer - its code base is not written in functional style, and it does need to handle unhandled exceptions from user code with some default behaviour, and i would argue it would make sense to tie specific behaviour to exception types. That is widely used and familiar pattern to many Java developers.
In addition - User Function already has a return value - for poll and produce flow. Of course it can be wrapped into Result object with status of execution and any data returned, but it just makes it more complex.

So - while i agree that there are cases when it is better and more natural to use return object with different statuses for success / failure modes - i do not think that is a good fit for Parallel Consumer code base.

@rkolesnev
Copy link
Member

Keeping it open - as i believe it would be beneficial to implement - bandwidth permitting.
Related enhancement that was closed as stale - but may have useful work in it already done / partially done - #291

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants