Allows readResponse() to throw exception

[tvhung83]

Context

Sometimes, there are exceptions during deserialization HTTP response: stream is closed, wrong expectation, etc.

Sample implementation

    @Override
    public TsResponse readResponse(Response response)
    {
        if (!response.hasEntity()) {
            return null;
        }
        try {
            InputStream out = response.readEntity(InputStream.class);
            filter.parse(new InputSource(out));
            return (TsResponse) unmarshallerHandler.getResult();
        }
        catch (JAXBException | SAXException | IOException e) {
            throw new DataException("Unexpected response: " + e.getMessage());
        }
    }

Example exceptions

Caused by: org.embulk.spi.DataException: Unexpected response: Premature EOF
    at org.embulk.output.myplugin.rest.jaxrs.TsResponseEntityReader.readResponse(TsResponseEntityReader.java:86)
    at org.embulk.output.myplugin.rest.jaxrs.TsResponseEntityReader.readResponse(TsResponseEntityReader.java:26)
    at org.embulk.util.retryhelper.jaxrs.JAXRSRetryHelper$1.call(JAXRSRetryHelper.java:96)
    at org.embulk.spi.util.RetryExecutor.run(RetryExecutor.java:100)
    at org.embulk.spi.util.RetryExecutor.runInterruptible(RetryExecutor.java:77)
    at org.embulk.util.retryhelper.jaxrs.JAXRSRetryHelper.requestWithRetry(JAXRSRetryHelper.java:78)
    at org.embulk.output.myplugin.rest.jaxrs.JAXRSService.retrySend(JAXRSService.java:368)
    at org.embulk.output.myplugin.rest.jaxrs.JAXRSService.publishDataSource(JAXRSService.java:276)
    at org.embulk.output.myplugin.MyPluginOutputPluginDelegate.egestEmbulkData(MyPluginOutputPluginDelegate.java:268)
    at org.embulk.output.myplugin.MyPluginOutputPluginDelegate.egestEmbulkData(MyPluginOutputPluginDelegate.java:62)
    at org.embulk.base.restclient.RestClientOutputPluginBaseUnsafe.resume(RestClientOutputPluginBaseUnsafe.java:51)
    at org.embulk.base.restclient.RestClientOutputPluginBase.resume(RestClientOutputPluginBase.java:62)
    at org.embulk.base.restclient.RestClientOutputPluginBaseUnsafe.transaction(RestClientOutputPluginBaseUnsafe.java:43)
    at org.embulk.base.restclient.RestClientOutputPluginBase.transaction(RestClientOutputPluginBase.java:56)
    at org.embulk.exec.BulkLoader$4$1$1.transaction(BulkLoader.java:560)
    ...

Caused by: org.embulk.spi.DataException: Unexpected response: The declaration for the entity "HTML.Version" must end with '>'.
    at org.embulk.output.myplugin.rest.jaxrs.TsResponseEntityReader.readResponse(TsResponseEntityReader.java:86)
    at org.embulk.output.myplugin.rest.jaxrs.JAXRSService.retrySend(JAXRSService.java:382)
    at org.embulk.output.myplugin.rest.jaxrs.JAXRSService.signIn(JAXRSService.java:168)
    at org.embulk.output.myplugin.rest.jaxrs.JAXRSService.<init>(JAXRSService.java:79)
    at org.embulk.output.myplugin.MyPluginOutputPluginDelegate.validateOutputTask(MyPluginOutputPluginDelegate.java:165)
    at org.embulk.output.myplugin.MyPluginOutputPluginDelegate.validateOutputTask(MyPluginOutputPluginDelegate.java:62)
    at org.embulk.base.restclient.RestClientOutputPluginBaseUnsafe.transaction(RestClientOutputPluginBaseUnsafe.java:42)
    at org.embulk.base.restclient.RestClientOutputPluginBase.transaction(RestClientOutputPluginBase.java:56)
    at org.embulk.exec.BulkLoader$4$1$1.transaction(BulkLoader.java:560)
    at org.embulk.exec.LocalExecutorPlugin.transaction(LocalExecutorPlugin.java:54)
    at org.embulk.exec.BulkLoader$4$1.run(BulkLoader.java:555)
    at org.embulk.spi.util.Filters$RecursiveControl.transaction(Filters.java:96)
    at org.embulk.spi.util.Filters.transaction(Filters.java:49)
    ...

Target

This will allow retryExecutor to handle the exception, instead of JAXRSResponseReader to catch exception and retry itself.

[tvhung83]

All the user plugins have to change their code in their plugins.

Ah, you're right, but I think users only have to change plugin code if they upgrade version.

Can you clarify the more detailed scenario why we really need this?

This is when readResponse() have temporary issues, such as connector received unexpected response, for example, HTML instead of XML. Or the stream was closed during reading.

I don't think this is a very reasonable change.

I've double checked jetty9x-retryhelper, they all have throws during deserialization:

• https://github.com/embulk/embulk-base-restclient/blob/master/embulk-util-retryhelper-jetty93/src/main/java/org/embulk/util/retryhelper/jetty93/Jetty93ResponseReader.java#L13-L15
• https://github.com/embulk/embulk-base-restclient/blob/master/embulk-util-retryhelper-jetty92/src/main/java/org/embulk/util/retryhelper/jetty92/Jetty92ResponseReader.java#L13-L15

[dmikurube]

Okay, it sounds possibly reasonable because Jetty93 and Jetty92 are doing similar, but what I'm still not sure is why we need to throw the Exception out of the readResponse method, not to process the Exception in readResponse. Can you share the scenario why?

[tvhung83]

@dmikurube It would make JAXRSResponseReader complicated unnecessarily, ie. having passing all retry params and manage its own retryExecutor, while it can just throw the exception and let JAXRSRetryHelper do its job.

[dmikurube]

@tvhung83 Can you show that by example code?

[tvhung83]

@dmikurube which example do you mean? Example of unnecessary complex JAXRSResponseReader?

[dmikurube]

Yes, I have no ideas about the "complicated unnecessarily".

[tvhung83]

Well, I PR to avoid the adhoc workaround, I'm not sure how I can explain it clearer, but I hope below can give you a hint:

• With throws:

  @Override
  public TsResponse readResponse(Response response) throws Exception
  {
      if (!response.hasEntity()) {
          return null;
      }
      InputStream out = response.readEntity(InputStream.class);
      filter.parse(new InputSource(out));
      return (TsResponse) unmarshallerHandler.getResult();
  }

• Without throws:

  @Override
  public TsResponse readResponse(Response response)
  {
      if (!response.hasEntity()) {
          return null;
      }
      try {
          InputStream out = response.readEntity(InputStream.class);
          filter.parse(new InputSource(out));
          return (TsResponse) unmarshallerHandler.getResult();
      }
      catch (SAXException | JAXBException | IOException e) {
          // retry here, and in order to do that, you will have another `retryExecutor` here
      }
  }

[tvhung83]

Thank you for reviewing 🙇 I also missed the need of requester. Could you help merge it when you got a chance?

[dmikurube]

Okay, merging.

We may want the new release to have an updated second level number of the version number since it may break the compatibility. (0.5.0 => 0.6.0)