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 support for remote API #186

Merged
merged 4 commits into from Oct 22, 2014

Conversation

Projects
None yet
3 participants
@rinrinne
Copy link
Member

commented Oct 16, 2014

This change adds remote API support. You can get the status of Gerrit connection using the below url:

$JENKINS_URL/plugin/gerrit-trigger/api/(xml|json)

Sample:

<pluginImpl>
  <server>
    <connected>true</connected>
    <frontEndUrl>http://localhost:8080/</frontEndUrl>
    <hostName>localhost</hostName>
    <name>gerrit1</name>
    <noConnectionOnStartup>false</noConnectionOnStartup>
    <sshPort>29418</sshPort>
    <timeoutWakeup>false</timeoutWakeup>
    <userName>foobar</userName>
    <httpUserName>foobar</httpUserName>
  </server>
</pluginImpl>

Also some small fixes:

  • No keep plugin instance in PluginImpl
  • Use WriteOnArrayList as server list (improve UI performance)

rinrinne added some commits Oct 16, 2014

Remove instance field in Plugin class
Jenkins creates only one instance from Plugin class then manage it.
So the field which keeps own instance is not needed.

Instead, we should use getPlugin(Class<T> clazz) in Jenkins class.
Change the implementation of server list to CopyOnWriteList
The purpose of this change is to throw away some synchronized methods.

Plugin instance is accessed from a lot of threads. So synchronized
methods block many processes. It would become a critical issue
if implement remote API.

This change solves such situation by using CopyOnWriteList.
It will improve UI performance.
Add remote API
This change adds remote API support.

You can get the status of Gerrit server using belowi url:

$JENKINS_URL/plugin/gerrit-trigger/api/
@rinrinne

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2014

BTW, if you plan to official release shortly, why don't you release one more beta for feature freeze before that? Let's check help contents.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Oct 16, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 20, 2014

Yes, It's been going on for too long now. I hope that beta-5 will be the final, and if people sign of on it that would be the 2.12 release.
New features will hopefully not come in until final release of 2.12

@@ -198,7 +207,7 @@ public synchronized void setServers(LinkedList<GerritServer> servers) {
* @param s the server to be added.
* @return the list after adding the server.
*/
public synchronized LinkedList<GerritServer> addServer(GerritServer s) {
public List<GerritServer> addServer(GerritServer s) {

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 20, 2014

Member

I think this breaks binary compatibility with previous versions?

Ithink there is an @WithBridgeMethod annotation you can use, if its applicable here.

This comment has been minimized.

Copy link
@rinrinne

rinrinne Oct 20, 2014

Author Member

Sure, so I will update tomorrow.

Today, I found such annotation because I faced issue regarding inner class visibility with Jenkins 1.554..

This comment has been minimized.

Copy link
@rinrinne

rinrinne Oct 21, 2014

Author Member

Maybe it is difficult to keep bc because LinkedList has List interface but no interface class for First/Last methods(e.g. getFirst/getLast, removeFirst/removeLast).

@WithBridgeMethods can generate bridge method, but it is done without any type check. It means that incompatible cast from List to LinkedList can be accepted by this annotation.

Unfortunately, as far as we use LinkedList as return value, it is no good solution in Java standard concurrency helper...

  • CopyOnWriteArrayList implements List interface only.
  • Same as Collections.synchronizedList wrapper

But it is useful not to use synchronized methods in PluginImpl. So we can take the below options:

  • Revert
  • Accept this change
  • Move server list and all synchronized methods to another class.

If possible, I want to allow this bc broken change. Because PluginImpl is not a exported interface.

WDYT?

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 22, 2014

Member

I think this change is OK, I just wanted to try to avoid this type of change if it is possible, but there is no big harm here I hope.
I was supposed to have stopped a method that returns specific List implementations when it showed up to be gin with, but I was too lazy at the time of merging it.

rsandell added a commit that referenced this pull request Oct 22, 2014

@rsandell rsandell merged commit 899f8f6 into jenkinsci:master Oct 22, 2014

1 check passed

default This pull request looks good
Details

@rinrinne rinrinne deleted the rinrinne:export-api branch Nov 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.