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 2021/08/25 20:48:43 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2248: Make Compactable file checks log and return empty
milleruntime opened a new pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248
* Move file checks outside of constructor of Files object and make them
log and return instead of throwing exceptions
* Threads were being killed quite frequently because they fail file
checks on seemingly common occurrences
* Also only make one copy of the runningJobs and pass it to Files constructor
* Closes #2239
--
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 a change in pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#discussion_r700233583
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/Compactable.java
##########
@@ -68,7 +68,7 @@ public Files(SortedMap<StoredTabletFile,DataFileValue> allFiles,
this.candidates = Collections.unmodifiableSet(candidates.stream()
.map(stf -> new CompactableFileImpl(stf, allFiles.get(stf))).collect(Collectors.toSet()));
- this.compacting = Set.copyOf(running);
+ this.compacting = runningJobsCopy;
Review comment:
I think its ok to leave this as `this.compacting = Set.copyOf(running);`. I just looked at the impl of Set.copyOf and it looks like the following.
```java
static <E> Set<E> copyOf(Collection<? extends E> coll) {
if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
return (Set<E>)coll;
} else {
return (Set<E>)Set.of(new HashSet<>(coll).toArray());
}
}
```
So when its something created by Set.of or Set.copyOf, is seems copyOf will not copy. Leaving the copyOf call in the code currently avoids a second copy while being robust to changes outside the constructors.
--
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] milleruntime commented on pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-910410984
The impl of Set.of() takes care of this for us so this PR is obsolete.
--
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] Manno15 commented on pull request #2248: Make Compactable file checks log and return empty
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-909731756
Changes made in #2252 seem to have fixed the issue in #2239. I still think some of the changes here are useful to keep, specifically moving the sanity check out of the files constructor and reducing to only one copy of runningJobs.
--
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] ctubbsii commented on pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-923564376
I removed the 2.1 version because this was closed and not merged.
--
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] milleruntime commented on pull request #2248: Make Compactable file checks log and return empty
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-910213010
> Changes made in #2252 seem to have fixed the issue in #2239. I still think some of the changes here are useful to keep, specifically moving the sanity check out of the files constructor and reducing to only one copy of runningJobs.
I agree there is some usefulness to changes here. I do feel like we should leave the exceptions in the constructor though since they served their original purpose of getting our attention. I don't want a future bug to go unnoticed in the debug logging. I will redo this PR.
--
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] milleruntime closed pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248
--
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 a change in pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#discussion_r700233583
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/Compactable.java
##########
@@ -68,7 +68,7 @@ public Files(SortedMap<StoredTabletFile,DataFileValue> allFiles,
this.candidates = Collections.unmodifiableSet(candidates.stream()
.map(stf -> new CompactableFileImpl(stf, allFiles.get(stf))).collect(Collectors.toSet()));
- this.compacting = Set.copyOf(running);
+ this.compacting = runningJobsCopy;
Review comment:
I think its ok to leave this as `this.compacting = Set.copyOf(running);`. I just looked at the impl of Set.copyOf and it looks like the following.
```java
static <E> Set<E> copyOf(Collection<? extends E> coll) {
if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
return (Set<E>)coll;
} else {
return (Set<E>)Set.of(new HashSet<>(coll).toArray());
}
}
```
So if it when its something created by Set.of or Set.copyOf, is seems copyOf will not copy. Leaving the copyOf call in the code currently avoids a second copy while being robust to changes outside the constructors.
--
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] milleruntime commented on pull request #2248: Make Compactable file checks log and return empty
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-906517768
> From a testing standpoint, these changes look good. I did test them locally and ran into no issues. It is still interesting how often that sanity check fails but it is better now that it is a debug statement and not something that kills the thread.
I agree. I tested them as well and things seem to run more smoothly. My only concern is that this change is masking an underlying bug that is causing the check to fail.
> I like the move of the sanity checks out of the files constructor and doing only one copy of runningJobs.
At the very least I think we can keep these changes. The question I have is: do we want to keep failing loudly because there is a bug or is it OK to fail gracefully.
I am going to leave this open for now until I can confirm with @keith-turner or find an answer to that question.
--
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] milleruntime commented on a change in pull request #2248: Make Compactable File use copy of runningJobs
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#discussion_r700339638
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/Compactable.java
##########
@@ -68,7 +68,7 @@ public Files(SortedMap<StoredTabletFile,DataFileValue> allFiles,
this.candidates = Collections.unmodifiableSet(candidates.stream()
.map(stf -> new CompactableFileImpl(stf, allFiles.get(stf))).collect(Collectors.toSet()));
- this.compacting = Set.copyOf(running);
+ this.compacting = runningJobsCopy;
Review comment:
Oh OK cool. Thanks for looking into the impl.
--
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] Manno15 commented on pull request #2248: Make Compactable file checks log and return empty
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2248:
URL: https://github.com/apache/accumulo/pull/2248#issuecomment-909731756
Changes made in #2252 seem to have fixed the issue in #2239. I still think some of the changes here are useful to keep, specifically moving the sanity check out of the files constructor and reducing to only one copy of runningJobs.
--
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