onClose callback for LogWatch #6876
Conversation
Chr031
requested review from
manusa,
oscerd,
rohanKanojia and
shawkins
as code owners
|
For better tracking, and probably some discussion, this will need an issue created.
I think that is the case when you pass in an OutputStream. If you instead just get the InputStream off of the LogWatch, you will see an exception or -1 when attempting to read past when it is done. |
|
Yes exactly, I was using an output stream.
Receiving the information by using directly the input stream of the LogWatch has from my point of view two issues:
- it implies the use for every LogWatch of a separated thread. This means also that you fall into blocking methods, and cannot easily scale if you want to follow many logs.
- it is not obvious to receive the closing event.
Let me know if you also need some unit tests. I didn’t implement any.
… Le 12 févr. 2025 à 00:25, Steven Hawkins ***@***.***> a écrit :
For better tracking, and probably some discussion, this will need an issue created.
I did not see any easy way to be informed of this event and thus decided to implement it.
I think that is the case when you pass in an OutputStream.
If you instead just get the InputStream off of the LogWatch, you will see an exception or -1 when attempting to read past when it is done.
—
Reply to this email directly, view it on GitHub <#6876 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPKZ7ENTXNVNQ53CHDBFXD2PKBF7AVCNFSM6AAAAABW6HJDC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJSGI4DMMBXGU>.
You are receiving this because you authored the thread.
|
There is still another thread involved - see the usage of the SerialExecutor - it's from the client's thread pool so it scales better. Ideally even when supplying what to write into, it could be non-blocking: #4154
Yes, that is an issue unless you are using virtual threads. The blocking nature of the InputStream is just a wrapper around the underlying non-blocking support - we just never came to a concensus on what to expose as a non-blocking api. An old draft of some possible ideas that wouldn't bring in additional dependencies: #5119
Ideally there should be one, and please still open an issue - that will also need referenced in the changelog. |
| * this exception will be passed as parameter, null otherwise | ||
| * @param onCloseAction | ||
| */ | ||
| void onClose(Consumer<Throwable> onCloseAction); |
There was a problem hiding this comment.
I would prefer
CompletionStage<Throwable> getDone()
And leave it up to the caller how to use that, rather than accepting / tracking a single consumer.
There was a problem hiding this comment.
Ok, why not this pattern.
I will still prefer the onClose name for the function.
CompletionStage<Throwable> onClose()
or
CompletionStage<Throwable> streamEnds()
could be acceptable from your point of view ?
There was a problem hiding this comment.
Sure, the method name can be whatever makes the most sense to you.
|
I just created an issue : #6880 |
Chr031
force-pushed
the
dev
branch
2 times, most recently
from
b924512 to
2ffb5a1
Compare
|
Chr031
force-pushed
the
dev
branch
2 times, most recently
from
097fced to
5f9533a
Compare
Description
Fixes #6880
This feature implements a listener on the closing event of the LogWatch class.
Currently, when a LogWatch is instantiated, and when the underlying stream is closed, the log processing stops and the underlying output stream received no more events without been inform that a dead end has been reach.
With the onClose action I added, when the stream processing ends, (with an error or not), the information can be forwarded and a post processing can be easily started.
Due to some timeouts, I was loosing the log stream and could not reconnect in a smooth way. I did not see any easy way to be informed of this event and thus decided to implement it.
Type of change
test, version modification, documentation, etc.)
Checklist