search

embulk / embulk-base-restclient

Base class library for Embulk plugins to access RESTful services
https://www.embulk.org/
Apache License 2.0
6 stars 7 forks source link

Create listener per request. closes #110. #111

Closed shroman closed 4 years ago

shroman commented 5 years ago

Currently the listener in *ResponseEntityReader is initialized once and reused for every subsequent request. However the listener is to be used per request, otherwise it fails to read from the response stream (it is closed after each request). This fix treats the listener per request and renames (deprecates for now) getListener to newListener to give an idea to the developer that it creates a new listener.

shroman commented 5 years ago

@dmikurube thanks, updated.

dmikurube commented 5 years ago

What is the case that creating *ResponseEntityReader every time is no good?

Yeah, it's "useful" to reuse the reader instance with a new listener. But at the same time, the caller needs much care so that another unaware code (written by others) must not call the newListener method out of the caller.

shroman commented 5 years ago

@dmikurube The issue is in the listener. Once it is used to read from the InputStream, it is not supposed to be reused for subsequent retries.

*ResponseEntityReader is initialized only once per Jetty92RetryHelper#requestWithRetry. I see no issue with that, this is how it is designed to be used. (Actually, if it has to be reinstantiated on every retry, it's not not very useful and can be merged with Jetty93SingleRequester into a kind of handler. <- and this will address your concern about calling newListener out of the caller, because listener becomes a requester's instance variable. But that's public API change, and probably has to be done in the next iteration).

dmikurube commented 5 years ago

Hmm, sorry, still not understanding the problem. You mean "retry" is not successfully retrying? What is the problem that is practically happening?

I understood listener is not reusable, but why not recreating *ResponseEntityReader does not work instead?

(Sorry if I'm saying stupid. But I remember nothing about this library's internal. Please do not expect any context shared with me...)

dmikurube commented 5 years ago

Ah, just in case, you don't need to take much care of compatibility in this library.

This library is loaded with the plugin, not on the core side. Even if its interfaces changes when a plugin developer upgrades this library, it just fails to compile, and the developer can fix the plugin. Compile-time issues on plugin side are much easier and acceptable than runtime/load-time issues with the core side... ;)

shroman commented 5 years ago

What is the problem that is practically happening?

  1. Use RetryHelper
  2. Fail on the 1st request (here InputStream is closed)
  3. Attempt to request again (getting the same listener with closed InputStream https://github.com/embulk/embulk-base-restclient/blob/master/embulk-util-retryhelper-jetty92/src/main/java/org/embulk/util/retryhelper/jetty92/Jetty92RetryHelper.java#L105) No data is read.
shroman commented 5 years ago

why not recreating *ResponseEntityReader does not work instead? you have to pass it here https://github.com/embulk/embulk-base-restclient/blob/master/embulk-util-retryhelper-jetty92/src/main/java/org/embulk/util/retryhelper/jetty92/Jetty92RetryHelper.java#L91

It is reused as-is by RetryExecutor.

It is possible to recreate it inside RetryExecutor.Retryable, but there must be some way to set timeoutMillis for *ResponseEntityReader (https://github.com/embulk/embulk-base-restclient/blob/master/embulk-util-retryhelper-jetty92/src/main/java/org/embulk/util/retryhelper/jetty92/StringJetty92ResponseEntityReader.java#L18). This is also not a clean solution.

shroman commented 5 years ago

the caller needs much care so that another unaware code (written by others) must not call the newListener method out of the caller.

If this is a show-stopper, I will refactor *ResponseEntityReader and *SingleRequester into one class to be in sync (one listener per request). But that will be API-breaking changes (I was thinking to do it for the next iteration).

dmikurube commented 5 years ago

Got it. Then "retry" is not successfully retrying, right?

If you think this is a tentative quick change, and you take care of compatibility, one safe way may be creating another *ResponseEntityReader class that allows renewing the listener.

Or, as said, you don't need to take much care of compatibility in this library. For example, you don't need to keep the getListener method, update the version to 0.8.0 (update the second digit), and leave a changelog what is changed.

If it is removed, it fails to compile, and developers are easily aware that the API has changed. They read the changelog/document, and catch up in the right way.

If it is kept just deprecated, developers may go as-is without taking a look. It may cause a worse situation that is found at runtime.

shroman commented 5 years ago

If you think this is a tentative quick change, and you take care of compatibility, one safe way may be creating another *ResponseEntityReader class that allows renewing the listener.

well, this one renews the listener (without forcing any changes) :) Or you think of something explicit like recreateListener() method?

Anyway, if you think this implementation is not a way to go (and I agree it not perfect), I will provide a new solution.

dmikurube commented 4 years ago

Ah, sorry, I forgot about this PR for a very long time... Anyway, embulk-util-retryhelper has been moved to https://github.com/embulk/embulk-util-retryhelper.

Let me close this now, and let's recreate this in https://github.com/embulk/embulk-util-retryhelper if needed.