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.

Sign up for GitHub

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

Jump to bottom

Chain the cause exception and log in case of failure #159

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Chain the cause exception and log in case of failure #159

merged 1 commit into from
Dec 18, 2017

Conversation

batmat
Copy link
Member

@batmat batmat commented Sep 14, 2017

My org folder scanning is failing, and I don't know why.

ERROR: [Thu Sep 14 13:26:12 UTC 2017] Could not fetch sources from navigator org.jenkinsci.plugins.github_branch_source.GitHubSCMNavigator@416977f4
hudson.AbortException: It seems https://api.github.com is unreachable
	at org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.checkApiUrlValidity(GitHubSCMSource.java:1022)
	at org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.retrieve(GitHubSCMSource.java:825)
	at jenkins.scm.api.SCMSource._retrieve(SCMSource.java:355)
	at jenkins.scm.api.SCMSource.fetch(SCMSource.java:309)
	at jenkins.branch.MultiBranchProjectFactory$BySCMSourceCriteria.recognizes(MultiBranchProjectFactory.java:263)
	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.recognizes(OrganizationFolder.java:1297)
	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.complete(OrganizationFolder.java:1312)
	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:256)
	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:206)
	at org.jenkinsci.plugins.github_branch_source.GitHubSCMNavigator.visitSources(GitHubSCMNavigator.java:961)
	at jenkins.branch.OrganizationFolder.computeChildren(OrganizationFolder.java:412)
	at com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:276)
	at com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:165)
	at jenkins.branch.OrganizationFolder$OrganizationScan.run(OrganizationFolder.java:863)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:419)
[Thu Sep 14 13:26:12 UTC 2017] Finished organization scan. Scan took 48 sec

From https://twitter.com/githubstatus there doesn't seem to be an issue right now. So overall, having more data to diagnose such issue in the future would be definitely helpful.

@reviewbybees

abayer
abayer approved these changes Sep 14, 2017
View reviewed changes
Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

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

lgtm

@ghost
Copy link

ghost commented Sep 14, 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.

recena
recena reviewed Sep 14, 2017
View reviewed changes
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java Outdated
@@ -1019,6 +1019,9 @@ private void checkApiUrlValidity(GitHub github) throws IOException {
github.checkApiUrlValidity();
} catch (HttpException e) {
String message = String.format("It seems %s is unreachable", apiUri == null ? GITHUB_URL : apiUri);
AbortException abortException = new AbortException(message);
abortException.initCause(e);
LOGGER.log(Level.WARNING, "Issue while checking GitHub URL validity", e);
Copy link
Contributor

@recena recena Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why not throw abortException instead of throw new AbortException(message);?

Copy link
Member

Choose a reason for hiding this comment

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

AbortException is designed to not have stacktraces and etc. It should clearly say what fails. But seems here @batmat trying to use it in a wrong way.
The right way is to print informative errors to listener (not stacktraces) and then fail with AbortException.

Copy link
Member

@KostyaSha KostyaSha Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

and @recena right, this code wouldn't even work. just a dead store that should fail from findbugs.

Copy link
Member

Choose a reason for hiding this comment

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

AbortException should be thrown (no printing to log) when there is a user-level mistake. It was not appropriate here IMO. Should be something like

} catch (HttpException e) {
    String message = String.format("It seems %s is unreachable", apiUri == null ? GITHUB_URL : apiUri);
    throw new IOException(message, e);
}

Copy link
Member

Choose a reason for hiding this comment

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

Should be throw new IOException.

The critical part is that we should throw an exception. AbortException should, in general, be reserved for aborting the job (at least from plugin code). IOException and InterruptedException should be thrown from the SCMSource implementations during scanning if any IO error or if interrupted respectively.

jglick
jglick requested changes Oct 4, 2017
View reviewed changes
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java Outdated
@@ -1019,6 +1019,9 @@ private void checkApiUrlValidity(GitHub github) throws IOException {
github.checkApiUrlValidity();
} catch (HttpException e) {
String message = String.format("It seems %s is unreachable", apiUri == null ? GITHUB_URL : apiUri);
AbortException abortException = new AbortException(message);
abortException.initCause(e);
LOGGER.log(Level.WARNING, "Issue while checking GitHub URL validity", e);
Copy link
Member

Choose a reason for hiding this comment

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

AbortException should be thrown (no printing to log) when there is a user-level mistake. It was not appropriate here IMO. Should be something like

} catch (HttpException e) {
    String message = String.format("It seems %s is unreachable", apiUri == null ? GITHUB_URL : apiUri);
    throw new IOException(message, e);
}

stephenc
stephenc requested changes Oct 5, 2017
View reviewed changes
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java Outdated
@@ -1019,6 +1019,9 @@ private void checkApiUrlValidity(GitHub github) throws IOException {
github.checkApiUrlValidity();
} catch (HttpException e) {
String message = String.format("It seems %s is unreachable", apiUri == null ? GITHUB_URL : apiUri);
AbortException abortException = new AbortException(message);
abortException.initCause(e);
LOGGER.log(Level.WARNING, "Issue while checking GitHub URL validity", e);
Copy link
Member

Choose a reason for hiding this comment

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

Should be throw new IOException.

The critical part is that we should throw an exception. AbortException should, in general, be reserved for aborting the job (at least from plugin code). IOException and InterruptedException should be thrown from the SCMSource implementations during scanning if any IO error or if interrupted respectively.

@batmat batmat force-pushed the more-diagnostics-when-no-url-validity branch from 0251d10 to e307bb5 Compare December 15, 2017 09:01
@batmat batmat requested review from jglick and stephenc December 15, 2017 09:02
@batmat
Copy link
Member Author

batmat commented Dec 15, 2017

@KostyaSha @recena @jglick @stephenc comments addressed. Thanks!

@stephenc stephenc merged commit e8c6c77 into jenkinsci:master Dec 18, 2017
@batmat batmat deleted the more-diagnostics-when-no-url-validity branch December 18, 2017 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KostyaSha KostyaSha KostyaSha left review comments

@recena recena recena approved these changes

@jglick jglick jglick approved these changes

@abayer abayer abayer approved these changes

@stephenc stephenc Awaiting requested review from stephenc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@batmat @abayer @jglick @stephenc @KostyaSha @recena