Fix #358 - Fixes deleteRecursive fails with ConnectionPoolTimeoutException#359
Fix #358 - Fixes deleteRecursive fails with ConnectionPoolTimeoutException#359dekobon merged 24 commits intoTritonDataCenter:masterfrom
Conversation
Additionally, we resolve a number of compiler warnings.
… big directory structures
…x for reading private fields.
|
|
||
| this.uriSigner = new UriSigner(this.config, keyPair, signer); | ||
|
|
||
| this.beanSupervisor = new MantaMBeanSupervisor(); |
There was a problem hiding this comment.
I'm unsure how this connects to the fork-join related changes.
There was a problem hiding this comment.
I messed up while fixing the merge conflict that was present this morning, there's now an extra assignment that needs to be removed.
| /* We repetitively run the find() -> delete() stream operation and check | ||
| * the diretory targeted for deletion by attempting to delete it this | ||
| * deals with unpredictable directory contents changes and occasional | ||
| * delays in Manta where it will return from a file delete operation, |
There was a problem hiding this comment.
Is this intended server side behavior? If not, is there a MANTA issue?
There was a problem hiding this comment.
This is intermittent observed behavior. I honestly don't have a lot of time to go deep and do the reproduction steps needed to file an issue that can actually be acted upon. Past work has shown me that is a multi-day process.
I'm just going to delete the comment so that it doesn't lead anyone down a bad path.
|
|
||
| /* If we get a directory not empty error we sleep | ||
| * every retry in the hopes that the subdirectory | ||
| * objects have been deleted by another process. */ |
There was a problem hiding this comment.
What other process could there be? Did you mean the other fork-join threads?
| } | ||
| } | ||
|
|
||
| /* We choose the small of the maximum number of connections minus two or |
| /* We choose the small of the maximum number of connections minus two or | ||
| * the number of available processors as the size of our fork join pool | ||
| * for find() parallelism. */ | ||
| final int parallelism = Math.min(config.getMaximumConnections() - 2, |
There was a problem hiding this comment.
Don't we need to guarantee that the result is a positive integer (or that the max connections are > 2)?
|
|
||
| /** | ||
| * Factory class that returns a {@link ForkJoinPool} instance configured with | ||
| * the proper number of maximum threads. |
There was a problem hiding this comment.
"proper" here, "correct" below. I think this javadoc would be more direct if it explained that the pool size is base on the number of connections, and that threads > connections is a problem.
|
|
||
| int processors = Runtime.getRuntime().availableProcessors(); | ||
|
|
||
| // Overwrite the default if the user has explicitly set a parallelism value |
There was a problem hiding this comment.
What is the case when having threads > connections would be an intentional and desirable configuration? I'm not sure I see what I user can do with this besides accidentally shoot themselves in the foot.
| * to make sure that the number of concurrent threads will not exceed | ||
| * the maximum number of available connections. | ||
| */ | ||
| private final ForkJoinPool findForkJoinPool; |
There was a problem hiding this comment.
Per some of the discussion in #316, should this class be package private?
There was a problem hiding this comment.
I reread #316, but I'm not understanding why we would want to change the scope from private to package private. Wouldn't that increase the surface area for compatibility problems?
| this.delete(obj.getPath()); | ||
| /* We repetitively run the find() -> delete() stream operation and check | ||
| * the diretory targeted for deletion by attempting to delete it this | ||
| * deals with unpredictable directory contents changes and occasional |
There was a problem hiding this comment.
"unpredictable directory contents"
Is this referring to concurrent modifications?
There was a problem hiding this comment.
Yes, it is. I will write that specifically.
| * this should be an empty directory. */ | ||
| delete(obj.getPath()); | ||
|
|
||
| if (LOG.isTraceEnabled()) { |
There was a problem hiding this comment.
nit: I don't think obj.getPath() is so expensive that it merits a double level guard.
There was a problem hiding this comment.
You are right it is ugly. I prefer the guard clause because obj.getPath() might become more expensive and then someone would have to remember to add guard clauses to all usages.
Honestly, that is a weakly held position that I hold with no passion.
|
EDIT: Looks like this is just a matter of performance, redacting comment |
|
This is great feedback and exactly what I was looking for. I'll be making improvements and pinging you both soon. |
…inPool parallelism calculation into separate functions.
|
This PR seems to share its first commit with #360. Would it make sense to incorporate remaining commits in that PR into this one so we can avoid a merge conflict? |
tjcelaya
left a comment
tjcelaya
left a comment
There was a problem hiding this comment.
Tests seem to indicate that there are no more ConnectionPoolTimeoutExceptions are being triggered by deleteRecursive but now we're seeing a lot of 500s instead. I'll file a separate ticket for those but I think this PR is ready to merge.
3 participants
No description provided.