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

Reschedule inflight pushes and help improvements for draft published #188

Merged
merged 2 commits into from Oct 30, 2014

Conversation

Projects
None yet
4 participants
@scoheb
Copy link
Contributor

commented Oct 27, 2014

With high volume repos, it is possible for Gerrit to re-schedule a replication
due to an inflight push. This re-schedule will result in a failed replication state
and will result in a failed build. By only proceeding on a successful replication
status, this will be avoided.

This commit also includes a fix to cope with missed events due to a Jenkins restart

Added a warning note for the usage of Draft Published events
when using Replication Events. The note is only seen when replication
is configured at the admin level.

Upgraded to gerrit-events 2.4.2

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

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

@@ -105,11 +105,16 @@ public CauseOfBlockage canRun(Item item) {
if (blockedItem.canRunWithTimeoutCheck()) {
if (blockedItem.replicationFailedMessage != null) {
item.addAction(new ReplicationFailedAction(blockedItem.replicationFailedMessage));
logger.trace(blockedItem.getEventDescription() + " -> " + blockedItem.replicationFailedMessage);

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 28, 2014

Member

please use logger.trace("{} -> {}", descr, message) formatting instead, so that string concatenation is avoided when trace level is turned off.


RepositoryModifiedEvent repositoryModifiedEvent = (RepositoryModifiedEvent)gerritCause.getEvent();
String eventDesc = getEventDescription(gerritCause.getEvent());
logger.trace(eventDesc);

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 28, 2014

Member

feels more like a debug level than trace level to me.

@@ -21,5 +21,16 @@
~ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
~ THE SOFTWARE.
-->
<j:jelly xmlns:j="jelly:core">
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<j:choose>

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 28, 2014

Member

A bit too many whitespaces here maybe?

@@ -316,6 +327,40 @@ public static ChangeMerged createChangeMergedWithPatchSetDate(String serverName,
event.setProvider(new Provider(serverName, "gerrit", "29418", "ssh", "http://gerrit/", "1"));
return event;
}

/**
* Gives you a ChangeMerged mock.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 28, 2014

Member

ChangeMerged -> DraftPublished

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 29, 2014

Member

There is still this little javadoc error.

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

More logging is good, but please go over them again and see if they are relevant and has appropriate level.

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

I'm feeling a bit lost (probably due to jet-lag) on how the impact on the rest of the replication logic would be, so I'll also wait for a +1 from @hugares before I hit the merge button ;)

@scoheb scoheb force-pushed the scoheb:replication-fixes branch from 9626cae to ce8fad9 Oct 28, 2014

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

You have 2 Checkstyle violations

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

Will push a change to fix this...sorry.

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

Also still a few places left where logging can be improved with parameters. The levels seems OK now I think.

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

I'll double-check!

@scoheb scoheb force-pushed the scoheb:replication-fixes branch from ce8fad9 to a7f05ea Oct 29, 2014

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

Changes made to some more logging calls to use parameters. Fixed checkstyle errors.

Will ask @hugares to review now...

@hugares

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

+1

escoheb added some commits Oct 17, 2014

escoheb
rescheduled inflight pushes + events not in cache
With high volume repos, it is possible for Gerrit to re-schedule a replication
due to an inflight push. This re-schedule will result in a failed replication state
and will result in a failed build. By only proceeding on a successful replication
status, this will be avoided.

This commit also includes a fix to cope with missed events due to a Jenkins restart
escoheb
Improve help for draft published
Added a warning note for the usage of Draft Published events
when using Replication Events. The note is only seen when replication
is configured at the admin level.

Upgraded to gerrit-events 2.4.2

@scoheb scoheb force-pushed the scoheb:replication-fixes branch from a7f05ea to 1c93b6f Oct 30, 2014

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2014

Fixed last javadoc error

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

Merge pull request #188 from scoheb/replication-fixes
Reschedule inflight pushes and help improvements for draft published

@rsandell rsandell merged commit dd3827e into jenkinsci:master Oct 30, 2014

1 check passed

default This pull request looks good
Details
@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 30, 2014

Are we set to release 2.12-beta-5 now?

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2014

We are set! 👍

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.