You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/07/31 17:43:53 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3671: Logs extent and files on failure to update tablets files
keith-turner opened a new pull request, #3671:
URL: https://github.com/apache/accumulo/pull/3671
(no comment)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1688501829
> Wouldn't these get logged elsewhere as well? I suppose in this case, redundant logging isn't important.
Its not guaranteed. I can't remember where, but there was some code in Accumulo that was scheduling futures on an execturor and never checking the result of the future. This type of situation would silently drop any exceptions. Also we can analyze all of Accumulo's code for exception handling, but anytime we pass control off to external code its possible it could drop an exception. So for this case like you said the redundant logging is not too big of an issue, want to make sure we see the problem.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1686977210
@ctubbsii all three try blocks currently can only throw RuntimeException. Two of the methods containing the try blocks throw IOException. So if I change the catch blocks to RuntimeException then its possible that someone in the future could call a method that throws IOException and it will compile but not be logged. Because of this I would prefer to leave the code as catching exception. Your suggestions makes sense for the case where the methods are not throwing checked exceptions already.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1686989152
@ctubbsii I experimented with adding a new checked exception to the code to see what would happen and it did not compile. Below is an example.
```java
void method1() throws IOException {
// some code that throws IOException
try{
// if a new code is added that throws a new checked exception that is not IOException then this code will not compile because the throw e later in the code can not compile. For example the following will not compile
if(true) throw new AccumuloException();
} catch(Exception e) {
log.error(e);
throw 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3671:
URL: https://github.com/apache/accumulo/pull/3671
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1659082549
I am not sure of the best way to to test this change. The only thing I could think of is manually modifying the code for one time test and adding some code that throws a run time exception. These changes would likely not be exercised by any of the ITs.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1686964490
@ctubbsii actually I think I understand what you are suggesting, I can try to make that 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3671: Logs extent and files on failure to update tablets files
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3671:
URL: https://github.com/apache/accumulo/pull/3671#issuecomment-1686960503
@ctubbsii I am not sure of the actual change you are suggesting. Would you mind pusing a commit or making a code suggestion?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org