You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "kevinrr888 (via GitHub)" <gi...@apache.org> on 2023/12/27 15:36:06 UTC
[PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
kevinrr888 opened a new pull request, #4118:
URL: https://github.com/apache/accumulo/pull/4118
Changes
- Added getSelectedFiles() method to CompactionConfigurer.InputParameters
- Added method functionality in the two implementations of this: ShellCompactCommandConfigurerTest and CompactableUtils
- Tested functionality in new IT: testGetSelectedFilesForCompaction() in CompactionIT
- Added new method getCompressionType() to PrintBCInfo (used in the test)
- Changed several method signatures to pass around necessary info for getSelectedFiles() implementations
- Renamed 'files' variable to 'inputFiles' in CompactableUtils and CompactableImpl for a more clear distinction between inputFiles and the (new) selectedFiles.
Potentially still left TODO:
- Add this functionality for external compactions?
- Move CompressionTypeConfigurer to it's own class?
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#issuecomment-1876145806
> Add this functionality for external compactions?
If you added the new method to all impls of CompactionConfigurer then I would think this case is already covered. However I did not actually follow the code to be sure.
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#discussion_r1437708696
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/PrintBCInfo.java:
##########
@@ -67,6 +67,18 @@ public void printMetaBlockInfo() throws IOException {
}
}
+ public String getCompressionType() throws IOException {
+ FSDataInputStream fsin = fs.open(path);
Review Comment:
Good point. I also don't see a reason why it shouldn't be in a try-with-resources. I moved both occurrences.
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#issuecomment-2018054340
@EdColeman thanks for pointing these out. The external compactions todo has been completed in this issue (I originally wasn't sure if my changes also worked for external compactions, but they do). The todo about moving the class probably isn't needed and is probably better suited for where it is now since it's currently only used in CompactionIT.
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#issuecomment-2005329061
@kevinrr888 - there are some todo's in the description that seem like they might be worth while for someone to pursue. Have those been captured in an issue?
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#discussion_r1437155059
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/PrintBCInfo.java:
##########
@@ -67,6 +67,18 @@ public void printMetaBlockInfo() throws IOException {
}
}
+ public String getCompressionType() throws IOException {
+ FSDataInputStream fsin = fs.open(path);
Review Comment:
```suggestion
FSDataInputStream fsin = fs.open(path);
```
Can this be added to the try-with-resources block? I see the another instance in this class where it could be added but I'm also not sure if it is outside of it on purpose.
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#issuecomment-1877368936
> > Add this functionality for external compactions?
>
> If you added the new method to all impls of CompactionConfigurer then I would think this case is already covered. However I did not actually follow the code to be sure.
Okay thanks! I did add the new method to the implementations of CompactionConfigurer.
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4118:
URL: https://github.com/apache/accumulo/pull/4118
--
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
Re: [PR] Added Indication of Intermediate Compactions to CompactionConfigurer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4118:
URL: https://github.com/apache/accumulo/pull/4118#discussion_r1441132325
##########
core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionConfigurer.java:
##########
@@ -59,6 +60,26 @@ public interface InputParameters {
public Collection<CompactableFile> getInputFiles();
+ /**
+ * For user and selector compactions:
+ * <ul>
+ * <li>Returns the selected set of files to be compacted.</li>
+ * <li>When getInputFiles() (inputFiles) and getSelectedFiles() (selectedFiles) are equal, then
+ * this is the final compaction.</li>
+ * <li>When they are not equal, this is an intermediate compaction.</li>
+ * <li>Intermediate compactions are compactions whose resultant RFile will be consumed by
+ * another compaction.</li>
+ * <li>inputFiles and selectedFiles can be compared using: <code>
+ * selectedFiles.equals(inputFiles instanceof Set ? inputFiles : Set.copyOf(inputFiles))
+ * </code></li>
+ * </ul>
+ * For system compactions:
+ * <ul>
+ * <li>There is no selected set of files so the empty set is returned.</li>
+ * </ul>
+ */
Review Comment:
This is a new API method so needs a since tag.
```suggestion
*
* @since 3.1
*/
```
--
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