jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Francesco Mari (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (OAK-4608) Unify background operation impls
Date Wed, 27 Jul 2016 08:46:20 GMT

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

Francesco Mari edited comment on OAK-4608 at 7/27/16 8:46 AM:
--------------------------------------------------------------

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.name());
                }
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
        }
    }
}
{code}

What do you think?


was (Author: frm):
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)

Mime
View raw message