[jira] [Commented] (OAK-4608) Unify background operation impls

["Francesco Mari (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-4608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15395253#comment-15395253 ] 

Francesco Mari commented on OAK-4608:
-------------------------------------

Everything that reduces code footprint is always welcome, but I personally don't like the inheritance relationship between {{PeriodicOperation}} and {{TriggeredOperation}}. It might be a personal preference, but I liked them to be distinct classes with distinct public interfaces. The methods {{trigger()}} and {{start()}} made clear that they had different usage patterns.

Also, including a fixed timeout in the implementation of the {{close()}} method makes that method tied to what we consider to be a good timeout for the use case of the {{FileStore}}. Maybe a different solution that would still have the benefits of your proposal would be to create an interface for both {{PeriodicOperation}} and {{TriggeredOperation}}:

{code}
interface BackgroundOperation {
    String name();
    boolean stop(long timeout, TimeUnit timeUnit) throws InterruptedException;
}

class TriggeredOperation implements BackgroundOperation {
    ...
}

class PeriodicOperation implements BackgroundOperation {
    ...
}
{code}

You can then create a utility method to convert a {{BackgroundOperation}} into a {{Closeable}}:

{code}
static Closeable asCloseable(BackgroundOperation op, long timeout, TimeUnit unit) {
    return new Closeable() {
        @Override
        public void close() throws IOException {
            try {
                if (op.stop(timeout, unit)) {
                    log.debug("{} was successfully shut down", op.name());
                } else {
                    log.warn("{} takes too long to shutdown", op.getName());
                }
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
        }
    }
}
{code}

What do you think?

> Unify background operation impls
> --------------------------------
>
>                 Key: OAK-4608
>                 URL: https://issues.apache.org/jira/browse/OAK-4608
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: segment-tar
>            Reporter: Alex Parvulescu
>            Assignee: Alex Parvulescu
>
> I unified the 2 background operation implementations and made then extend {{Closeable}}, this reduces the code footprint a bit and makes it easier for me to add a new operation.
> [~frm], [~mduerig] change is in this commit, please review!
> https://github.com/stillalex/jackrabbit-oak/commit/01b81cc068db8200fa736d96bd3abb064f988590



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)