Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add core types to be used to replace the use of jcr.Session in the kernel API #1107

Merged
merged 13 commits into from Oct 13, 2016
Merged

Add core types to be used to replace the use of jcr.Session in the kernel API #1107

merged 13 commits into from Oct 13, 2016

Conversation

acoburn
Copy link
Contributor

@acoburn acoburn commented Sep 30, 2016

I have a complete implementation for this, but so as not to make the PR too massive, these are the new interfaces being added which will be used to replace instances of jcr.Session, jcr.Repository, and the existing org.fcrepo.kernel.api.services.TransactionService.

Begins to address: https://jira.duraspace.org/browse/FCREPO-1868

Supersedes #968


public interface State {}

public enum RequiredState implements State {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what does it mean for a FedoraSession to be in ACTIVE as opposed to BATCH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACTIVE is something that is used in the current impl to test jcr.Session::isLive. BATCH is a marker to indicate that a particular session persists across multiple HTTP requests.

Copy link
Contributor

@ajs6f ajs6f Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do that here? Can't we just offer begin, commit, etc., and leave it at that? HTTP notions don't belong here, to my mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean remove ACTIVE? I have that here b/c the http layers test for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying I don't see the need for this state machinery at all.


/**
* Determine whether the session has the provided state
* @param <T> an enum that extends the default State enum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot extend a Java enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is wrong -- State is an interface, not an enum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what is the point of the generic? Doesn't it come to:

boolean hasState(State state);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's correct, I can simplify that signature.

* @param prefix the prefix
* @param uri the URI
*/
void setNamespacePrefix(String prefix, String uri);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any real data-handling methods, but we're still going to demand that you manage namespaces? I don't think this makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that part can go.

* @return the default timeout value
*/
public static Duration operationTimeout() {
if (System.getProperty(TIMEOUT_SYSTEM_PROPERTY) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about justjava.lang.System.getProperty(String, String)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that would be much better

* @author acoburn
* @since Sept 30, 2016
*/
public interface BatchService {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a kind of "manage the connections between users and open transactions" thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently an interface called TransactionService. This interface is a slight modification of that, mostly to remove some of the baggage of using the word "transaction"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I wonder if we want this stuff in the kernel at all.

Partly, this is about the fact that I don't think we have any clear sense of why there are all these *Service guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason they are in the kernel is so that the HTTP layers can access these "services" via spring injection. Moving those out would be a structural change that goes way beyond the scope of this effort.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm not asking you to do that. I just don't want to make any effort to preserve them if we can avoid it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, leave it be for now. I'm certainly not trying to block this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajs6f: related to the org.fcrepo.kernel.api.services.*Service types, I wonder whether they might not be better situated in a different package -- say: org.fcrepo.spi or org.fcrepo.kernel.spi (which could be independent of the fcrepo-kernel-api module). Once we have a full specification and remove all of the JCR concepts from the API, I think there's a certain amount of re-organization that can happen w/r/t the modules and package namespaces, but that's a conversation for later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's get through this. I have yet to hear anyone give a purpose for those types. (I know you have OSGi in mind, but I don't think could have been our original purpose. If we decide to have these kinds of types for OSGi, they should be impl'd in a common OSGi-related module, not in the kernel-api forcing everyone to impl them directly.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that the purpose of these types is to make it possible for the HTTP layer (fcrepo-http-api and fcrepo-http-commons) to access those implementation-specific services that are injected via Spring (but that could be substituted with Blueprint for OSGi). Either way, I agree that they are not a necessary part of the kernel-api.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do that, but they are only needed to do that on the assumption that model types (like FedoraResource) aren't managed by the bean container or other outer system. @awoods and I have talked in the past about whether that's a step we ought to contemplate taking. My point here is that whatever we do in the JCR impl, other people may want to play it out differently elsewhere and we shouldn't make their lives any harder.

* @param session The session to use for this batch operation
* @param username the name of the {@link java.security.Principal}
*/
void begin(FedoraSession session, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the FedoraSession interface, it would appear that a userId is already associated with sessions. How is this arg username reconciled with the session userId?

Copy link
Contributor Author

@acoburn acoburn Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct: the jcr.Session interface has a getUserID method, and the new FedoraSession interface has a getUserId method -- which in the impl just proxies to the jcr.Session::getUserID. The issue is that in the case of an anonymous user, the result of the jcr.Session::getUserID is not null, but rather <anonymous>. But we can't actually rely on that string, because that, too, is configurable in the repository.json file. This way, we are relying on the value of the java.security.Principal::getName value for looking up FedoraSession values.

Believe me -- my first stab at this excluded the username parameter on all of these methods, but relying on that conflicts with how transaction values are looked up.

* @param username the name of the {@link java.security.Principal}
* @return the {@link FedoraSession} with this user
*/
FedoraSession getSession(String txId, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected result if the txId exists but not with the provided arg username?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change the behavior of the existing TransactionServiceImpl, namely an Exception is thrown: https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/services/TransactionServiceImpl.java#L137-L138

Personally, I'd prefer to return an Optional, but this PR is about removing the jcr.Session, not about changing everything.

* @param username the name of the {@link java.security.Principal}
* @return the {@link FedoraSession} object for the defined user
*/
boolean exists(String txid, String username);
Copy link

@awoods awoods Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: change txid to txId for consistency.
..or the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go further and remove the "tx" part altogether, just using sessionId.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awoods I changed this and all subsequent txid names to sessionId.

* @param txid the Id of the {@link FedoraSession}
* @return the {@link FedoraSession} object
*/
default boolean exists(String txid) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: change txid to txId for consistency.
..or the other way around.

* @param txid the id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void commit(String txid, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: change txid to txId for consistency.
..or the other way around.

*
* @param txid the id of the {@link FedoraSession}
*/
default void commit(String txid) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...

* @param txid the id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void abort(String txid, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...

*
* @param txid the id of the {@link FedoraSession}
*/
default void abort(String txid) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...

@awoods
Copy link

awoods commented Oct 1, 2016

👍

@acoburn
Copy link
Contributor Author

acoburn commented Oct 2, 2016

@awoods, @ajs6f I added the modeshape implementation for these new interfaces. Once I get the green light on this, I'll add the rest of the commits (which actually remove the use of jcr.Session from the kernel-api).

*
* @return the JCR Repository
*/
public Repository getRepository() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming is confusing; maybe getJcrRepository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me. Also, I should double check the use of this method, because we may be able to do away with this altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajs6f I renamed the method here and in FedoraSessionImpl (getSession -> getJcrSession). The FedoraRepositoryImpl::getJcrRepository method is only used for unit tests -- I don't know if that's a good reason to keep it, but generally, I tried to keep existing unit/integration tests in place. All things being equal, I would probably remove that test and therefore that method, too. @awoods: thoughts?

@acoburn
Copy link
Contributor Author

acoburn commented Oct 3, 2016

@awoods @ajs6f let me know when you are ready for the full implementation. Barring any changes with the code so far, the next commit will be 76 files changed, 1019 insertions(+), 2175 deletions(-)

Instant getCreated();

/**
* Get the date this session expires
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've talked about immortal sessions. Either there should be a note explaining what to use for that "value" or this should be Optional<Instant> (I like the latter).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the FedoraSession interface to return an Optional<Instant>

* @param key the key
* @param value the value
*/
void setSessionData(String key, String value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about multi-valued keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-valued keys aren't supported. Should they be?

As a little background, the reason this method is here is so that the HTTP layers can record the Host and UserAgent headers in such a way that those are available to the eventing machinery: for example: https://git.io/vPZrr and https://git.io/vPZoe. In the current impl, the data is packed into a JSON structure and stored as a String. We could do that here as well (make the signature into setSessionData(String data), but I thought this provided a little more structure which could be relied on more than hoping the JSON string is well formatted, which is what we have now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about what I am going to do with this, not what you have already done with it: we're writing an API. IOW, I can definitely think of multi-valued use cases, but I'm not asking you to support them, just to write this method's signature in such a way as to avoid disallowing them. E.g.

void addSessionData(String key, String value);
Collection<String> getSessionData(String key);
void removeSessionData(String key, String value);
default removeSessionData(String key) {
    getSessionData(key).forEach(v -> removeSessionData(key, v));
}

or maybe just commit to Guava's MultiMap

// @return a _mutable_ multimap of session-related data
MultiMap<String, String> getSessionData();

throw new RepositoryRuntimeException(ex);
}
}
throw new IllegalArgumentException("Credentials are of the wrong type");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a more specific subtype?

if (repository instanceof FedoraRepositoryImpl) {
return ((FedoraRepositoryImpl)repository).getJcrRepository();
}
throw new IllegalArgumentException("FedoraRepository is of the wrong type");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto-- maybe a more specific exception?


public static final String TIMEOUT_SYSTEM_PROPERTY = "fcrepo.session.timeout";

private final Session session;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is a little confusing: maybe call it jcrSession or something?

if (session instanceof FedoraSessionImpl) {
return ((FedoraSessionImpl)session).getJcrSession();
}
throw new IllegalArgumentException("FedoraSession is of the wrong type");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a more specific exception type?


@Override
public FedoraSession getSession(final String sessionId, final String username) {
final FedoraSession tx = transactions.computeIfAbsent(getTxKey(sessionId, username), s -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Java 8 but this seems a bit fancy-shmancy. How about

String txKey = getTxKey(sessionId, username);
if (!transactions.containsKey(txKey)) {
    throw new SessionMissingException("Batch session with id: " + txKey + " is not available!");
}
return transactions.get(txKey);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for that structure has to do with the fact that another thread may remove the session after a containsKey check. We could, however, do something like:

FedoraSession s = transactions.get(txKey);
if (s == null) {
    throw new SessionMissingException("...");
}
return s;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadocs for Map::computeIfAbsent say The default implementation makes no guarantees about synchronization or atomicity properties of this method. so you are relying on ConcurrentHashMaps impl-specific behavior here.... but do we have any understanding anyway of the atomicity of *Service? I don't think this is coherent.


@Override
public void commit(final String sessionId, final String username) {
final FedoraSession session = transactions.remove(getTxKey(sessionId, username));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the session.commit() fails, don't you not want to have removed it from the transactions? The client might want to retry.


@Override
public void abort(final String sessionId, final String username) {
final FedoraSession tx = transactions.remove(getTxKey(sessionId, username));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above-- if the expire fails, shouldn't the session still be available for further detective work?

}

private static String getTxKey(final String sessionId, final String username) {
return username == null ? sessionId : username + ":" + sessionId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just me, but I would actually prefer anonymous sessions to have the colon.

@acoburn
Copy link
Contributor Author

acoburn commented Oct 5, 2016

@ajs6f I addressed the latest round of comments in that last commit.

@@ -77,16 +80,31 @@
Map<String, String> getNamespaces();

/**
* Set session-specific data
* Add session-specific data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a note here that it is up to the impl whether multiple values per key are afforded, and a note on the immediate impl that they aren't?

private final String id;
private final Instant created;
private final Map<String, String> sessionData;
private final ConcurrentHashMap<String, String> sessionData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we aren't claiming that this is thread-safe, what's the point of ConcurrentHashMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because if there are multiple concurrent requests in the context of the same transaction, there will be several threads writing to that Map. The question about whether the class is thread-safe at all has to do with how the jcr.Session handles concurrency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get that. If multiple threads are going to hit a single instance of this class, how is it okay for this class to not be thread-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it ought to be thread-safe. I just don't know if the jcr.Session actually is. In the current code, we're treating the JCR Session as if it is thread-safe, so one option is to keep the ConcurrentHashMap (or use a Collections::synchonizedMap) and remove the comment about thread-safety.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should make guarantees, and if we don't, we should use the cheapest Map that does the job.

throw new SessionMissingException("Batch session with id: " + s + " is not available");
});
return tx;
System.out.println(transactions.keySet().toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some debugging juice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops!

@acoburn
Copy link
Contributor Author

acoburn commented Oct 5, 2016

@ajs6f more comments addressed in the latest commit.

void begin(FedoraSession session, String username);

/**
* Create a new FedoraSession for the anonymous user and add it to the currently open ones
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by: "Create a new FedoraSession..." since a FedoraSession is being passed into this method.

* @param username the name of the {@link java.security.Principal}
* @return the {@link FedoraSession} with this user
*/
FedoraSession getSession(String sessionId, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if no such session for the provided username exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A SessionMissingException is thrown.

}

/**
* Refresh an existing session using an implementation-defined default
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between "Refresh an existing session" here and FedoraSession.updateExpiry? https://github.com/fcrepo4/fcrepo4/pull/1107/files#diff-2a974e96c1ebe0bcd554be7d704b6955R48

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BatchService implementation will typically involve an in-memory (possibly distributed) map of FedoraSessions. BatchService::refresh will update the expiry of one of those batch sessions. I.e., BatchService::refresh will probably call FedoraSession::updateExpiry, but as for how that is done is up to the implementation.

* @param sessionId the id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void commit(String sessionId, String username);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be some overlap between this interface and FedoraSession. How are we reconciling this commit with FedoraSession.commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BatchService::commit will call FedoraSession::commit, but it may also (depending on the implementation) remove the persistent session from the (distributed) session map.

*/
private static Map<String, FedoraSession> transactions = new ConcurrentHashMap<>();

public static final long REAP_INTERVAL = 1000;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is public (I believe) to facilitate testing. I can add the appropriate guava annotation.

public class FedoraSessionImpl implements FedoraSession {

// The default timeout is 3 minutes
public static final String DEFAULT_TIMEOUT = Long.toString(ofMinutes(3).toMillis());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above...

// The default timeout is 3 minutes
public static final String DEFAULT_TIMEOUT = Long.toString(ofMinutes(3).toMillis());

public static final String TIMEOUT_SYSTEM_PROPERTY = "fcrepo.session.timeout";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above...

private static final ObjectMapper mapper = new ObjectMapper();

/**
* Create a Fedora session with a JCR session and a username
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This javadoc should probably be updated to remove reference to username.

* @param key the data key
* @param value the data value
*
* Note: while the FedoraSession inteface permits multi-valued
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos: inteface and assciated

* @param key the data key
* @param value the data value
*
* Note: while the FedoraSession inteface permits multi-valued
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified that the current codebase does not require appending values to an existing UserData key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -51,8 +52,10 @@
public class FedoraSessionImpl implements FedoraSession {

// The default timeout is 3 minutes
@VisibleForTesting
public static final String DEFAULT_TIMEOUT = Long.toString(ofMinutes(3).toMillis());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not actually used in any testing code... and can presently be safely private.

public static final String DEFAULT_TIMEOUT = Long.toString(ofMinutes(3).toMillis());

@VisibleForTesting
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not actually used in any testing code... and can presently be safely private.


@VisibleForTesting
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not actually used in any testing code... and can presently be safely private.

@awoods
Copy link

awoods commented Oct 12, 2016

This is ready to go from my perspective. @ajs6f, have all of your comments been addressed?

@ajs6f
Copy link
Contributor

ajs6f commented Oct 13, 2016

@awoods I have no idea. That's not @acoburn's fault at all-- I find this new Github "review" mode terribly confusing and unhelpful and I can no longer keep track of this conversation. Frankly, if @acoburn gives us his word that he addressed everything, that's fine with me.

@acoburn
Copy link
Contributor Author

acoburn commented Oct 13, 2016

And I find that this "review mode" makes it difficult to keep track of issues that I've resolved. As far as I know, I have addressed all of the issues brought up in this discussion, but it has also involved a lot of back and forth, and it is entirely possible that something has fallen between the cracks.

@awoods
Copy link

awoods commented Oct 13, 2016

It sounds like, to the best of our collective knowledge, this PR is ready to go. Feel free to merge it @ajs6f .

@ajs6f
Copy link
Contributor

ajs6f commented Oct 13, 2016

Am I to close and reopen the ticket?

@acoburn
Copy link
Contributor Author

acoburn commented Oct 13, 2016

@ajs6f just reopen the Jira ticket.

@ajs6f ajs6f merged commit a11360d into fcrepo:master Oct 13, 2016
@acoburn acoburn deleted the fcrepo-1868 branch December 27, 2016 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants