You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/04/28 11:39:36 UTC
[GitHub] [solr] tboeghk opened a new pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
tboeghk opened a new pull request #99:
URL: https://github.com/apache/solr/pull/99
# Description
The `IndexFetcher` swallows Exceptions upon replication errors and does not log the stack trace properly. This led to some confusion when setting up a `TLOG/PULL` replica ensemble.
# Solution
This PR adds the exception's stack trace into the log messages upon replication abort. This will help users for further analysis.
# Tests
No tests needed (I guess)
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #99:
URL: https://github.com/apache/solr/pull/99#issuecomment-834569517
Unfortunately this PR originated from an organizational fork and not from a personal repo, so I cannot enable access for maintainers ([bug in Github](https://github.com/isaacs/github/issues/1681)).
But I happily added the change to `solr/CHANGES.txt`!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke merged pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
cpoerschke merged pull request #99:
URL: https://github.com/apache/solr/pull/99
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #99:
URL: https://github.com/apache/solr/pull/99#issuecomment-834651393
> Unfortunately this PR originated from an organizational fork and not from a personal repo, so I cannot enable access for maintainers ([bug in Github](https://github.com/isaacs/github/issues/1681)).
Ah, interesting, thanks for sharing!
> But I happily added the change to `solr/CHANGES.txt`!
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #99:
URL: https://github.com/apache/solr/pull/99#discussion_r628262149
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -677,12 +677,12 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
markReplicationStop();
return successfulInstall ? IndexFetchResult.INDEX_FETCH_SUCCESS : IndexFetchResult.INDEX_FETCH_FAILURE;
} catch (ReplicationHandlerException e) {
- log.error("User aborted Replication");
+ log.error("User aborted Replication", e);
return new IndexFetchResult(IndexFetchResult.FAILED_BY_EXCEPTION_MESSAGE, false, e);
} catch (SolrException e) {
throw e;
} catch (InterruptedException e) {
- throw new InterruptedException("Index fetch interrupted");
+ throw (InterruptedException)(new InterruptedException("Index fetch interrupted").initCause(e));
Review comment:
observation: changing the details of the exception being thrown here (not its type but it having or not having a cause or a particular type of cause) could theoretically change the calling code's behaviour. practically that seems unlikely though. also inspection of calling code finds no obvious signs that behaviour would change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #99:
URL: https://github.com/apache/solr/pull/99#issuecomment-834494687
Hmm, I just tried to merge in the latest `origin/main` and add the `solr/CHANGES.txt` entry before merging here but then github wouldn't let me push the commits to the pull request branch.
@tboeghk would you be comfortable enabling maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) for this PR branch? Or could I ask you to add the `solr/CHANGES.txt` entry e.g. something like
```
* SOLR-15377: Improve IndexFetcher's exception logging upon replication errors. (Torsten Bøgh Köster, Christine Poerschke)
```
in (say) the "Other Changes" sub-section of the 8.9.0 section? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on a change in pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
tboeghk commented on a change in pull request #99:
URL: https://github.com/apache/solr/pull/99#discussion_r623818060
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -677,12 +677,12 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
markReplicationStop();
return successfulInstall ? IndexFetchResult.INDEX_FETCH_SUCCESS : IndexFetchResult.INDEX_FETCH_FAILURE;
} catch (ReplicationHandlerException e) {
- log.error("User aborted Replication");
+ log.error("User aborted Replication", e);
return new IndexFetchResult(IndexFetchResult.FAILED_BY_EXCEPTION_MESSAGE, false, e);
} catch (SolrException e) {
throw e;
} catch (InterruptedException e) {
- throw new InterruptedException("Index fetch interrupted");
+ throw new InterruptedException("Index fetch interrupted", e);
Review comment:
You're absolutely right! Way better fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #99: [SOLR-15377] Do not swallow execption stack traces in IndexFetcher
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #99:
URL: https://github.com/apache/solr/pull/99#discussion_r623802940
##########
File path: solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
##########
@@ -677,12 +677,12 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
markReplicationStop();
return successfulInstall ? IndexFetchResult.INDEX_FETCH_SUCCESS : IndexFetchResult.INDEX_FETCH_FAILURE;
} catch (ReplicationHandlerException e) {
- log.error("User aborted Replication");
+ log.error("User aborted Replication", e);
return new IndexFetchResult(IndexFetchResult.FAILED_BY_EXCEPTION_MESSAGE, false, e);
} catch (SolrException e) {
throw e;
} catch (InterruptedException e) {
- throw new InterruptedException("Index fetch interrupted");
+ throw new InterruptedException("Index fetch interrupted", e);
Review comment:
Locally this does not compile for me on Java 11, maybe instead this?
```suggestion
throw (InterruptedException)(new InterruptedException("Index fetch interrupted").initCause(e));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org