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

Add close method in RecordBuffer #90

Closed muga closed 7 years ago

muga commented 7 years ago

This PR adds close method in RecordBuffer. The method is called in RestClientPageOutput#close(). A output plugin's RecordBuffer object sometimes has (owns) an external resource. In this case, the resource needs to be closed when RestClientPageOutput#close() is called.

muga commented 7 years ago

@dmikurube What do you think about this method adding? Please take a look.

dmikurube commented 7 years ago

@muga Basically looks good. One question is: do you think close is the best to do that, not finish?

muga commented 7 years ago

@dmikurube Thank you for your comment. I think, that's good point. That depends on the relation ship between a record buffer and the resources. Therefore, record buffer might need to have both of close and finish. What do you think about it?

sakama commented 7 years ago

Please ignore CI failure. I tried to enable Travis and noticed .travis.yml change is required. Created another PR https://github.com/embulk/embulk-base-restclient/pull/91

But JacksonTaskReportRecordBuffer change is needed?

muga commented 7 years ago

@sakama Yes, necessary. Thank you for your comment.

muga commented 7 years ago

@dmikurube I added a comment to describe that close and finish method needs to release the external resource that RecordBuffer owns. To be honest, I don't have good example that it needs to be released in the finish method. In almost all cases, close method only might be good enough but. What do you think about it?

@sakama I changed JacksonTaskReportRecordBuffer. Please take a look.

dmikurube commented 7 years ago

@muga Thanks, but the description may be a bit long. How about splitting that into methods' JavaDoc such as:

Or, it is possible to commit directly from {@code RecordBuffer}. But, the destination must accept parallel
uploads, and developers may need to take care of transactions and orders.

If {@code RecordBuffer} owns external resources for direct uploading, the resources need to be released in its
{@code close} or {@code finish} methods. Releasing resources in {@code close} is typical.
/**
 * Finishes the {@code RecordBuffer}.
 *
 * This method is called when {@code RestClientPageOutput#finish} is called. Implement this method typically to
 * finish resources managed in {@code RecordBuffer}.
 */
public abstract void finish();
/**
 * Closes the {@code RecordBuffer}.
 *
 * This method is called when {@code RestClientPageOutput#close} is called. Implement this method typically to
 * close resources managed in {@code RecordBuffer}.
 */
public abstract void close();
dmikurube commented 7 years ago

Merged.

muga commented 7 years ago

@dmikurube thank you!