You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/11/03 13:33:21 UTC
[GitHub] [accumulo] keith-turner commented on a change in pull request #1759: Wrap undo in try block
keith-turner commented on a change in pull request #1759:
URL: https://github.com/apache/accumulo/pull/1759#discussion_r516374929
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/tableOps/create/ChooseDir.java
##########
@@ -60,9 +63,16 @@ public long isReady(long tid, Master environment) {
@Override
public void undo(long tid, Master master) throws Exception {
- Path p = tableInfo.getSplitDirsPath();
- FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
- fs.delete(p, true);
+ // Clean up split files if ChooseDir operation fails
+ try {
+ if (tableInfo.getInitialSplitSize() > 0) {
+ Path p = tableInfo.getSplitDirsPath();
+ FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
+ fs.delete(p, true);
+ }
+ } catch (NullPointerException | IOException e) {
+ log.error("Failed to undo ChooseDir operation", e);
Review comment:
When on the receiving end of an exception like this its really nice to have some information that help correlates it with other activity in the system. However the danger of trying to get more information for an exception is that code made fail. I think the following is pretty safe and gives a good bit of helpful info.
```suggestion
var spdir = Optional.ofNullable(tableInfo).map(TableInfo::getSplitDirsPath).orElse(null);
log.error("{} Failed to undo ChooseDir operation, split dir {} ",FateTxId.format(tid), spdir, e);
```
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/tableOps/create/ChooseDir.java
##########
@@ -60,9 +63,16 @@ public long isReady(long tid, Master environment) {
@Override
public void undo(long tid, Master master) throws Exception {
- Path p = tableInfo.getSplitDirsPath();
- FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
- fs.delete(p, true);
+ // Clean up split files if ChooseDir operation fails
+ try {
+ if (tableInfo.getInitialSplitSize() > 0) {
+ Path p = tableInfo.getSplitDirsPath();
+ FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
+ fs.delete(p, true);
+ }
+ } catch (NullPointerException | IOException e) {
+ log.error("Failed to undo ChooseDir operation", e);
Review comment:
When on the receiving end of an exception like this its really nice to have some information that helps correlates it with other activity in the system. However the danger of trying to get more information for an exception is that code made fail. I think the following is pretty safe and gives a good bit of helpful info.
```suggestion
var spdir = Optional.ofNullable(tableInfo).map(TableInfo::getSplitDirsPath).orElse(null);
log.error("{} Failed to undo ChooseDir operation, split dir {} ",FateTxId.format(tid), spdir, e);
```
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/tableOps/create/ChooseDir.java
##########
@@ -60,9 +63,16 @@ public long isReady(long tid, Master environment) {
@Override
public void undo(long tid, Master master) throws Exception {
- Path p = tableInfo.getSplitDirsPath();
- FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
- fs.delete(p, true);
+ // Clean up split files if ChooseDir operation fails
+ try {
+ if (tableInfo.getInitialSplitSize() > 0) {
+ Path p = tableInfo.getSplitDirsPath();
+ FileSystem fs = p.getFileSystem(master.getContext().getHadoopConf());
+ fs.delete(p, true);
+ }
+ } catch (NullPointerException | IOException e) {
+ log.error("Failed to undo ChooseDir operation", e);
Review comment:
When on the receiving end of an error like this its really nice to have some information that helps correlates it with other activity in the system. However the danger of trying to get more information for an error is that the code made fail. I think the following is pretty safe and gives a good bit of helpful info.
```suggestion
var spdir = Optional.ofNullable(tableInfo).map(TableInfo::getSplitDirsPath).orElse(null);
log.error("{} Failed to undo ChooseDir operation, split dir {} ",FateTxId.format(tid), spdir, 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