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/12/20 20:21:56 UTC
[PR] deprecates table.file.max [accumulo]
keith-turner opened a new pull request, #4098:
URL: https://github.com/apache/accumulo/pull/4098
(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
Re: [PR] deprecates table.file.max [accumulo]
Posted by "FineAndDandy (via GitHub)" <gi...@apache.org>.
FineAndDandy commented on PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#issuecomment-1866798882
I would love if instead of deprecating this we actually made it an imperative to compact.
--
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] deprecates table.file.max [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner closed pull request #4098: deprecates table.file.max
URL: https://github.com/apache/accumulo/pull/4098
--
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] deprecates table.file.max [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#discussion_r1433613426
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1125,13 +1125,19 @@ public enum Property {
"The number of replicas for a table's RFiles in HDFS. When set to 0, HDFS"
+ " defaults are used.",
"1.3.5"),
+ @Deprecated(since = "2.1.3")
TABLE_FILE_MAX("table.file.max", "15", PropertyType.COUNT,
- "The maximum number of RFiles each tablet in a table can have. When"
- + " adjusting this property you may want to consider adjusting"
- + " table.compaction.major.ratio also. Setting this property to 0 will make"
- + " it default to tserver.scan.files.open.max-1, this will prevent a tablet"
- + " from having more RFiles than can be opened. Setting this property low may"
- + " throttle ingest and increase query performance.",
+ "This property is currently only used by the deprecated DefaultCompactionStrategy. This "
+ + "property was used by merging minor compaction in previous versions of Accumulo, "
+ + "but that feature was remove prior to the 2.1.0 release. The way "
+ + "DefaultCompactionStrategy uses this property could result in a quadratic "
+ + "amount of compaction work happening over time. For example if a tablets "
+ + "smallest files is 1G and a new 1K files is added, if that new files exceeds "
+ + "this max it would compact the 1G and 1K file which is not ideal. The ideal "
+ + "way to control the number of tablets per file is by adjusting the "
+ + "table.compaction.major.ratio property because it will perform logarithmic "
+ + "amounts of compaction work over time versus the quadratic amount of work this "
+ + "property can cause. The new DefaultCompactionPlanner does not use this property.",
Review Comment:
I thought I had added a checkstyle rule awhile back to disallow double space after period, to force our string literals to be consistent with only a single space. They often are rendered in HTML, such as in our generated property docs, and since HTML collapses spaces, it can be a real pain to copy and paste something from the HTML property docs and try to grep for it on the command line when it fails to match because of mismatched space counts.
So, I thought I could quickly fix this for you, so we could merge this in, because I thought that was the problem with the build checks. However, I never actually committed that checkstyle rule, and it turns out it wasn't the problem with the build.
--
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] deprecates table.file.max [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#issuecomment-1867937514
> I would love if instead of deprecating this we actually made it an imperative to compact.
I have been thinking about how this could be achieved efficiently. The way the old code worked for max files was so inefficient. The compaction ratio efficiently reduces the number of files, but it is hard to reason about the number of files it will produce.
One problem with the compaction ratio is that the total number of files you end up with in a tablet depends on the smallest expected files For example if really small files are arriving in a tablet, like 1000 bytes. Then you will end up with 3x1K files, 3x3K files, 3x9K files, etc. So starting at 1K with a compaction ratio of 3, you end up with 20 levels or 60 total files before getting to 1G. So with the current compaction ratio the number of levels and total number files you end up with depends s on the smallest file arriving.
For larger files we want a higher compaction ratio, but maybe we don't want that for smaller files. This led me to wonder about making the compaction ratio a function of the file size. Something like the following that more aggressively reduces the number of small files, while still doing logarithmic work.
```java
// function that given a file size computes a compaction ratio
Function<Long,Double> ratioFunction = size -> {
if(size < 1_000) {
return 1.0;
} else if(size < 10_000) {
return 1.25;
} else if(size < 100_000) {
return 1.5;
}else if(size < 1_000_000) {
return 1.75;
} else if(size < 10_000_000) {
return 2.0;
} else if(size < 100_000_000) {
return 2.25;
} else {
return 3.0;
}
};
```
Tying to figure out if we could generate the above function given a target number of max files and expected max size that accomodates any minimum file size. So a user does not specify the above function, but one is generated based on desired max files and expected max file size. If this could be done, it could be implemented as another compaction planner for 2.x.
--
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] deprecates table.file.max [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#discussion_r1433583585
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1125,13 +1125,19 @@ public enum Property {
"The number of replicas for a table's RFiles in HDFS. When set to 0, HDFS"
+ " defaults are used.",
"1.3.5"),
+ @Deprecated(since = "2.1.3")
TABLE_FILE_MAX("table.file.max", "15", PropertyType.COUNT,
- "The maximum number of RFiles each tablet in a table can have. When"
- + " adjusting this property you may want to consider adjusting"
- + " table.compaction.major.ratio also. Setting this property to 0 will make"
- + " it default to tserver.scan.files.open.max-1, this will prevent a tablet"
- + " from having more RFiles than can be opened. Setting this property low may"
- + " throttle ingest and increase query performance.",
+ "This property is currently only used by the deprecated DefaultCompactionStrategy. This "
+ + "property was used by merging minor compaction in previous versions of Accumulo, "
+ + "but that feature was remove prior to the 2.1.0 release. The way "
+ + "DefaultCompactionStrategy uses this property could result in a quadratic "
+ + "amount of compaction work happening over time. For example if a tablets "
+ + "smallest files is 1G and a new 1K files is added, if that new files exceeds "
+ + "this max it would compact the 1G and 1K file which is not ideal. The ideal "
+ + "way to control the number of tablets per file is by adjusting the "
+ + "table.compaction.major.ratio property because it will perform logarithmic "
+ + "amounts of compaction work over time versus the quadratic amount of work this "
+ + "property can cause. The new DefaultCompactionPlanner does not use this property.",
Review Comment:
```suggestion
"This property is currently only used by the deprecated DefaultCompactionStrategy. This "
+ "property was used by merging minor compaction in previous versions of Accumulo, "
+ "but that feature was remove prior to the 2.1.0 release. The way "
+ "DefaultCompactionStrategy uses this property could result in a quadratic "
+ "amount of compaction work happening over time. For example if a tablets "
+ "smallest files is 1G and a new 1K files is added, if that new files exceeds "
+ "this max it would compact the 1G and 1K file which is not ideal. The ideal "
+ "way to control the number of tablets per file is by adjusting the "
+ "table.compaction.major.ratio property because it will perform logarithmic "
+ "amounts of compaction work over time versus the quadratic amount of work this "
+ "property can cause. The new DefaultCompactionPlanner does not use this property.",
```
--
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] deprecates table.file.max [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#issuecomment-1868538201
`Math.min(3,1+Math.floor(Math.log(size)/Math.log(10)-2)*.25)`
--
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] deprecates table.file.max [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#issuecomment-1875896746
Decided to not deprecate the table.file.max property in favor of #4127. However not going to close this PR for now, the changes to make all the test stop using the table.file.max prop in this PR still seem like good changes to make. So plan to make this no longer deprecate the prop, but still remove the unnecessary use of the prop in all of the test.
--
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] deprecates table.file.max [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4098:
URL: https://github.com/apache/accumulo/pull/4098#issuecomment-1911247419
> Decided to not deprecate the table.file.max property in favor of #4127. However not going to close this PR for now, the changes to make all the test stop using the table.file.max prop in this PR still seem like good changes to make. So plan to make this no longer deprecate the prop, but still remove the unnecessary use of the prop in all of the test.
The description update to document how it works with the default strategy would be good to keep from this as well, if that's not already done as part of 4127.
--
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