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

Expose invoke method & required interface for downstream plugins #520

Merged
merged 6 commits into from
Aug 4, 2017
Merged

Expose invoke method & required interface for downstream plugins #520

merged 6 commits into from
Aug 4, 2017

Conversation

kzantow
Copy link
Contributor

@kzantow kzantow commented Aug 3, 2017

Minor change to allow downstream plugins to access repository cache in a synchronous manner with appropriate locks.

This is necessary for enabling efficient and hopefully Jenkins-accurate git read/write in blueocean

@reviewbybees esp. @stephenc

Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

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

In making public, need javadocs with guidelines on acceptable usage

@@ -211,7 +211,7 @@ public boolean changesSince(@CheckForNull SCMRevision revision, @NonNull OutputS
}
}

/*package*/ interface FSFunction<V> {
public interface FSFunction<V> {
Copy link
Member

Choose a reason for hiding this comment

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

If making public we should provide clear guidelines in javadocs

@@ -147,7 +147,7 @@ public SCMFile getRoot() {
return commitId;
}

/*package*/ <V> V invoke(final FSFunction<V> function) throws IOException, InterruptedException {
public <V> V invoke(final FSFunction<V> function) throws IOException, InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

Javadocs

@kzantow
Copy link
Contributor Author

kzantow commented Aug 3, 2017

@stephenc done

@michaelneale
Copy link
Member

Javadocs LGTM and lovingly documented 🐝

@MarkEWaite
Copy link
Contributor

How does this pull request interact with #502 which seems to also be using the GitSCMFileSystem cache?

@kzantow
Copy link
Contributor Author

kzantow commented Aug 3, 2017

@MarkEWaite this is just exposing a way to get access to the git cache, should not have any impact on #502 either way

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Aug 3, 2017

But I thought that #502 extends the locking concept for the GitSCMFileSystem cache. Won't your calls through invoke() need to honor those same locks?

@ghost
Copy link

ghost commented Aug 4, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@kzantow
Copy link
Contributor Author

kzantow commented Aug 4, 2017

@MarkEWaite that's the whole point... calls through invoke lock the Git SCM cache

@kzantow
Copy link
Contributor Author

kzantow commented Aug 4, 2017

@MarkEWaite again, that's the whole point: invoke should be, well, invoked, during a Git SCM cache lock, just look at the code

* @param <V> return type
* @param function callback executed with a locked repository
* @return something
* @throws IOException
Copy link
Member

Choose a reason for hiding this comment

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

Java 8's javadocs will blow up for missing reasons on the exception

Copy link
Member

Choose a reason for hiding this comment

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

just change to

@throws IOException if there is an I/O error
@throws InterruptedException if interrupted

@stephenc
Copy link
Member

stephenc commented Aug 4, 2017

LGTM (apart from the java8 javadoc error for missing reason in the @throws)

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Aug 4, 2017

@kzantow thanks for the explanation, and for the javadoc. Once you address the comment from @stephenc, it is ready to merge.

@kzantow
Copy link
Contributor Author

kzantow commented Aug 4, 2017

@MarkEWaite @stephenc done

@stephenc stephenc merged commit 889aafb into jenkinsci:master Aug 4, 2017
@MarkEWaite MarkEWaite added the skip-changelog Exclude from the changelog label Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Exclude from the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants