Improve error handling on GROBID server connection issues #7026

nitram509 · 2020-10-18T13:37:17Z

Hi,

This PR relates to issues #6517 and #6891.
It's more improvement than an actual fix because the root cause is: the server is not available (which I can't fix).

Motivation:
When I read #6517 and #6891, I realized, that these were reported by 'power users',
means people who e.g. understand how to activate debug logging.
When I tried to reproduce the issues, I realized, that for the end-users the issue is not transparent.
Additionally, on my OSX, the Java 14 runtime has an infinite connect timeout set, so I never got a visual response on my action (refers to "New entry from plain text" button)

What I changed:

add a property connectTimeout to URLDownloader
- this avoids the situation, where e.g. on similar setups than mine (Java 14.0.2 on OSX), there's an infinite default connectTimeout and I never saw any error in UI nor log.
- set the default connectTimeout to 30 seconds. Which is e.g. the same order of magnitude as in many other tools and thus should have no side effects to other usages of this class
- set the connectTimeout for GROBID server to 5 seconds, because this should be sufficient as I expect this is a cloud-based service - please comment, if this assumption is wrong.
refactored the GrobidServce so that IOExceptions are percolating higher in the call-stack and then can be handled in the UI
in case of error, I add a notification to the user, with an explicit message, so users have a chance to know, what's going on.. IMHO, that's more correct and easier to understand for users, over the current behavior, where just a message "0 entries parsed" occurs. (screenshot below in the comments)
added EN and DE translation for the new message.

I hope, with this new behavior, users will better understand, why this feature does not work as expected.

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Any feedback welcome.

add information on GROBID server connection issue for user relates to JabRef#6517 and JabRef#6891

nitram509 · 2020-10-18T13:38:11Z

Here's a screenshot, how the error message looks like (the red arrow is just for demonstration purpose).

tobiasdiez

Thanks a lot for your PR. The code looks already very good. I've a few minor remarks. Could you please address them?

tobiasdiez · 2020-10-18T14:09:37Z

src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java

@@ -58,6 +64,16 @@ public StringProperty inputTextProperty() {
    public void startParsing() {
        BackgroundTask.wrap(() -> currentCitationfetcher.performSearch(inputTextProperty.getValue()))
                      .onRunning(() -> dialogService.notify(Localization.lang("Your text is being parsed...")))
+                      .onFailure((e) -> {
+                          if (e instanceof UncheckedIOException) {
+                              String msg = Localization.lang("There are connection issues with the %0 server. Detailed information: %1.",


I think it's not so important for the user which exact url failed (especially since the popup disappears to quickly to type it in the url of a webbrower to check). So I suggest the following wording: "Could not connect to JabRef server (Connection timed out)."

And maybe add the details via error log.

Fair point, @tobiasdiez.
This should be "Could not connect to GROBID server (Connection timed out)." Shouldn't it?

I would still say "JabRef server"...the user knows what Jabref is, but not necessarily what grobid is. I would see this also as an "implementation detail", maybe in the future we switch the backend from grobid to something else..

It was tricky to get the Message to a phrase as you proposed.
I reduced the level of detail towards:
"There are connection issues with a JabRef server. Detailed information: %0."

(See latest screenshot)

Is this OK for you @tobiasdiez ?

Perfect! Thanks

src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java

src/main/java/org/jabref/logic/net/URLDownload.java

src/test/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcherTest.java

use assertThrow is more convenient

reduce detail level from user message rework to use FetcherException instead of UncheckedIOException

nitram509 · 2020-10-18T16:02:25Z

All reviews incorporated.
May I ask for another review, please?

/cc @tobiasdiez

tobiasdiez

Thanks a lot for the quick follow-up! From my side, this is now good to go. We have the convention that at least two core developers have to approve a PR, so please have a bit of patience until the second review arrives.

@koppor The author check fails again...should we disable it again?

nitram509 · 2020-10-18T16:42:10Z

Thanks a lot for the quick follow-up! From my side, this is now good to go. We have the convention that at least two core developers have to approve a PR, so please have a bit of patience until the second review arrives.

@koppor The author check fails again...should we disable it again?

Thank you very much, @tobiasdiez
I'm participating in the Hacktoberfest.
May I ask, to label this PR as "hacktoberfest-accepted", to signal that I did follow the rules, please?

Siedlerchr · 2020-10-18T16:46:25Z

The PR wil count towards hacktoberfest because we applied the hacktoberfest label on the repo.

Siedlerchr · 2020-10-18T16:48:32Z

Thanks for your contribution! Looking foward seeing more from you ;)

add explicit configuration for connectTimeout in URLDownload

07f39de

add information on GROBID server connection issue for user relates to JabRef#6517 and JabRef#6891

nitram509 added 2 commits October 18, 2020 15:41

add CHANGELOG entry

07b6f5a

fix style issues detected by checkstyle

9abce44

nitram509 changed the title ~~[WIP] Improve error handling on GROBID server connection issues~~ Improve error handling on GROBID server connection issues Oct 18, 2020

tobiasdiez requested changes Oct 18, 2020

View reviewed changes

nitram509 marked this pull request as ready for review October 18, 2020 14:15

tobiasdiez added status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Oct 18, 2020

nitram509 added 3 commits October 18, 2020 16:25

use .toMilliseconds() is more convenient

98712e7

use assertThrow is more convenient

incorporate feedback from review

add6e3e

reduce detail level from user message rework to use FetcherException instead of UncheckedIOException

switch to debug level in order to reduce verboseness in the log

7ae5b57

tobiasdiez approved these changes Oct 18, 2020

View reviewed changes

tobiasdiez removed the status: changes required Pull requests that are not yet complete label Oct 18, 2020

Siedlerchr approved these changes Oct 18, 2020

View reviewed changes

Siedlerchr merged commit 4213a8d into JabRef:master Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling on GROBID server connection issues #7026

Improve error handling on GROBID server connection issues #7026

nitram509 commented Oct 18, 2020 •

edited

Loading

nitram509 commented Oct 18, 2020 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Oct 18, 2020

tobiasdiez Oct 18, 2020

nitram509 Oct 18, 2020

tobiasdiez Oct 18, 2020

nitram509 Oct 18, 2020 •

edited

Loading

tobiasdiez Oct 18, 2020

nitram509 commented Oct 18, 2020

tobiasdiez left a comment

nitram509 commented Oct 18, 2020

Siedlerchr commented Oct 18, 2020

Siedlerchr commented Oct 18, 2020

Improve error handling on GROBID server connection issues #7026

Improve error handling on GROBID server connection issues #7026

Conversation

nitram509 commented Oct 18, 2020 • edited Loading

nitram509 commented Oct 18, 2020 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Oct 18, 2020

Choose a reason for hiding this comment

tobiasdiez Oct 18, 2020

Choose a reason for hiding this comment

nitram509 Oct 18, 2020

Choose a reason for hiding this comment

tobiasdiez Oct 18, 2020

Choose a reason for hiding this comment

nitram509 Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

tobiasdiez Oct 18, 2020

Choose a reason for hiding this comment

nitram509 commented Oct 18, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

nitram509 commented Oct 18, 2020

Siedlerchr commented Oct 18, 2020

Siedlerchr commented Oct 18, 2020

nitram509 commented Oct 18, 2020 •

edited

Loading

nitram509 commented Oct 18, 2020 •

edited

Loading

nitram509 Oct 18, 2020 •

edited

Loading