You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/06/20 22:28:49 UTC
[GitHub] [cassandra] akin-tekeoglu opened a new pull request, #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
akin-tekeoglu opened a new pull request, #1696:
URL: https://github.com/apache/cassandra/pull/1696
I also fixed the `batch_size_warn_threshold ` test. It was checking against `column_index_size` before.
[Task](https://issues.apache.org/jira/browse/CASSANDRA-17650)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902597039
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
Nit: the final brace should be on a new line, K&R style:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName)
{
```
Also, the `byteValue` argument is meant to contain a value in kibibytes, so we should probably name it `kibibyteValue`. Or maybe just `kibibytes`, as it's named in the`DataStorageSpec.IntKibibytesBound` constructor that we use immediately below:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String configName)
{
```
I would also rename the `configName` argument to `propertyName`, I think:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String propertyName)
{
```
I wonder if we should put this new method and the related `checkValidForByteConversion` closer to each other in the file, wdyt?
One last thought about this is that perhaps we should move this validation into `DataStorageSpec.IntKibibytesBound`, maybe with a validation method or with a static constructor performing the validation. But we don't have to do that during this ticket.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r903092123
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
This is part of the new config format where users can provide unit of their choice. Now, they cannot provide at the moment any unit smaller than the one used internally by Cassandra for a respective property because during conversion there can be lost precision. Also, we will run into more issues with the accepted upper bound. So in the case we call `DataStorageSpec.IntKibibytesBound(10, MEBIBYTES)` which says - the user wants/provides 10Mebibytes for a property which was type Int and it is used in Kibibytes by Cassandra internally. So it can't be more than Integer.MAX_VALUE-1 in kibibytes. Please refer to the Java Doc and this document - https://cassandra.apache.org/doc/trunk/cassandra/new/configuration.html
`batch_size_fail_threshold `- It can't be more than `Integer.MAX_VALUE` in kibibytes. So the long widening when we return it in bytes can accommodate this. But currently `getBatchSizeFailThreshold()` uses the` IntKibibytesBound.toBytes()` which would return Int and this is a bug. We will need to create a new method `IntKibibytesBound.toBytesInLong()` to be used by `getBatchSizeFailThreshold() `
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r905640895
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -559,6 +559,7 @@ else if (conf.repair_session_space.toMebibytes() > (int) (Runtime.getRuntime().m
checkValidForByteConversion(conf.column_index_size, "column_index_size");
checkValidForByteConversion(conf.column_index_cache_size, "column_index_cache_size");
checkValidForByteConversion(conf.batch_size_warn_threshold, "batch_size_warn_threshold");
+ checkValidForByteConversion(conf.batch_size_fail_threshold, "batch_size_fail_threshold");
Review Comment:
As we opted in for restoring the widening, I believe this check is not needed and expected anymore. It will also reduce the established upper bound from the previous versions.
It should have upper bound Integer.MAX_VALUE-1 in kibibytes, not bytes and the kibibytes upper value is already handled in the constructor.
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -326,7 +326,8 @@ public void testExceptionsForInvalidConfigValues() {
fail("Should have received a ConfigurationException batch_size_warn_threshold = 2GiB");
}
catch (ConfigurationException ignored) { }
- Assert.assertEquals(4096, DatabaseDescriptor.getColumnIndexSize());
+ Assert.assertEquals(5120, DatabaseDescriptor.getBatchSizeWarnThreshold());
+
}
Review Comment:
I suggest we add below test in order to ensure no more regressions in the future in this area. We can also add a comment about that as Andres suggested on the ticket, to be more clear what is this and where it came from. (for legacy reasons we do widen to long instead of the check)
```
@Test
public void testWidenToLongInBytes() throws ConfigurationException
{
DatabaseDescriptor.setBatchSizeFailThresholdInKiB(2147483646);
Assert.assertEquals((2147483646L * 1024), DatabaseDescriptor.getBatchSizeFailThreshold());
}
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902822512
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
Not sure whether we want to cover all this in this one ticket or split
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902833880
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
One more thing on my mind. I think further to adding comment it will be great if we add some unit tests for the bounds which will prevent people from regressing again in the future. It is confusing and easy to miss
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] akin-tekeoglu commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
akin-tekeoglu commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r906200809
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -326,7 +326,8 @@ public void testExceptionsForInvalidConfigValues() {
fail("Should have received a ConfigurationException batch_size_warn_threshold = 2GiB");
}
catch (ConfigurationException ignored) { }
- Assert.assertEquals(4096, DatabaseDescriptor.getColumnIndexSize());
+ Assert.assertEquals(5120, DatabaseDescriptor.getBatchSizeWarnThreshold());
+
}
Review Comment:
Added 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic closed pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
URL: https://github.com/apache/cassandra/pull/1696
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] akin-tekeoglu commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
akin-tekeoglu commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r903060636
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
@ekaterinadimitrova2 @adelapena I have a couple of questions
1. Why do we have `DataStorageUnit` as a constructor parameter in subclasses of `DataStorageSpec`? E.g., Isn't `IntKibibytesBound` represents values like 1 KiB, 100 KiB. Why do we have an option like this `new DataStorageSpec.IntKibibytesBound(10, MEBIBYTES)`
2. Shouldn't the `quantity` of the constructor of IntBound classes be int? It is long right now.
3. What is the upper limit of `batch_size_fail_threshold`?
Either I missed the idea behind `DataStorageSpec`, or the class hierarchy is misleading.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902776217
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
I think the widening that was used before instead of validation was broken by the adoption of `DataStorageSpec.IntKibibytesBound.toBytes`, which returns an `int`. So we have to either add new conversion method or also use validation here. Probably the former for compatibility, although I'm not sure.
As for moving validation logic into `DataStorageSpec`, I was thinking not in moving the entire method as it is but the logic to verify that the value can be converted to a lesser unit without losing precision. Perhaps a method returning a boolean saying whether the conversion to a unit is going to produce precision loss, so the validator on `DatabaseDescriptor` can use it. That would be a single utility method on `DataStorageSpec`, not extending to their child classes. But as I mentioned, I think we don't have to do that, if we do it, here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] akin-tekeoglu commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
akin-tekeoglu commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r906200627
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -559,6 +559,7 @@ else if (conf.repair_session_space.toMebibytes() > (int) (Runtime.getRuntime().m
checkValidForByteConversion(conf.column_index_size, "column_index_size");
checkValidForByteConversion(conf.column_index_cache_size, "column_index_cache_size");
checkValidForByteConversion(conf.batch_size_warn_threshold, "batch_size_warn_threshold");
+ checkValidForByteConversion(conf.batch_size_fail_threshold, "batch_size_fail_threshold");
Review Comment:
Updated
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -559,6 +559,7 @@ else if (conf.repair_session_space.toMebibytes() > (int) (Runtime.getRuntime().m
checkValidForByteConversion(conf.column_index_size, "column_index_size");
checkValidForByteConversion(conf.column_index_cache_size, "column_index_cache_size");
checkValidForByteConversion(conf.batch_size_warn_threshold, "batch_size_warn_threshold");
+ checkValidForByteConversion(conf.batch_size_fail_threshold, "batch_size_fail_threshold");
Review Comment:
Removed the check
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] akin-tekeoglu commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
akin-tekeoglu commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r905359381
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
I tried to fix the issues that you mentioned before as well. Please retake a look.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902729732
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
batch_size_fail_threshold doesn't need the validation and I assumed this part of the patch will be removed.
Anyway, I am not against adding a nice utility method and agree with @adelapena's review comments.
> One last thought about this is that perhaps we should move this validation into DataStorageSpec.IntKibibytesBound, maybe with a validation method or with a static constructor performing the validation. But we don't have to do that during this ticket.
Didn't we intentionally leave it out of those classes and opted out of custom static methods and constructors as it is a corner case for only a few parameters? I thought we didn't want to encourage too much customizations in those classes to ensure we keep them clean, only in case we consider this will be a very common case.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902833880
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
One more thing on my mind. I think further to adding comment it will be great if we add some unit tests for the bounds of those properties which will prevent people from regressing again in the future. It is confusing and easy to miss
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902597039
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
Nit: the final brace should be on a new line, K&R style:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName)
{
```
Also, the `byteValue` argument is meant to contain a value in kibibytes, so we should probably name it `kibibyteValue`. Or maybe just `kibibytes`, as it's named in the`DataStorageSpec.IntKibibytesBound` constructor that we use immediately below:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String configName)
{
```
I would also rename the `configName` argument to `propertyName`, I think:
```suggestion
private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int kibibytes, String propertyName)
{
```
I wonder if we should put this new method and the related `checkValidForByteConversion` closer to each other in the file, wdyt?
One last thought about this is that perhaps we should move this validation into `DataStorageSpec.IntKibibytesBound`, maybe with a validation method or with static constructor performing the validation. But we don't have to do that during this ticket.
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -326,7 +326,24 @@ public void testExceptionsForInvalidConfigValues() {
fail("Should have received a ConfigurationException batch_size_warn_threshold = 2GiB");
}
catch (ConfigurationException ignored) { }
- Assert.assertEquals(4096, DatabaseDescriptor.getColumnIndexSize());
Review Comment:
Good catch!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902821882
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
Thankfully we don't need to handle the precision issue here as going to smaller unit we use multiplication. Overflow is the only issue we need to handle.
Yes, both` DataStorageSpec.IntKibibytesBound` and `DataStorageSpec.IntMebibytesBound` will need something like `toBytesInLong`.
It seems that `batch_size_fail_threshold` is the only parameter of type `IntKibibytesBound` that suffers from this issue.
Now, I went ahead and did a further check for the other properties that are widen to long as it seems this bug was introduced when we removed the static methods from `DataStorageSpec`.
We will need to add `toBytesInLong` also for `IntMebibytesBound` to cover
```
getMinFreeSpacePerDriveInBytes() / min_free_space_per_drive,
compaction_large_partition_warning_threshold / getMinFreeSpacePerDriveInBytes(), getMaxHintsFileSize() / max_hints_file_size
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1696: CASSANDRA-17650: Add validation for batch_size_fail_threshold
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1696:
URL: https://github.com/apache/cassandra/pull/1696#discussion_r902821882
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1656,14 +1653,18 @@ public static int getUnloggedBatchAcrossPartitionsWarnThreshold()
public static void setBatchSizeWarnThresholdInKiB(int threshold)
{
- DataStorageSpec.IntKibibytesBound storage = new DataStorageSpec.IntKibibytesBound(threshold);
- checkValidForByteConversion(storage, "batch_size_warn_threshold");
- conf.batch_size_warn_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_warn_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_warn_threshold");
}
public static void setBatchSizeFailThresholdInKiB(int threshold)
{
- conf.batch_size_fail_threshold = new DataStorageSpec.IntKibibytesBound(threshold);
+ conf.batch_size_fail_threshold = createIntKibibyteBoundAndEnsureItIsValidForByteConversion(threshold,"batch_size_fail_threshold");
+ }
+
+ private static DataStorageSpec.IntKibibytesBound createIntKibibyteBoundAndEnsureItIsValidForByteConversion(int byteValue, String configName){
Review Comment:
Thankfully we don't need to handle the precision issue here as going to smaller unit we use multiplication. Overflow is the only issue we need to handle.
Yes, both` DataStorageSpec.IntKibibytesBound` and `DataStorageSpec.IntMebibytesBound` will need something like `toBytesInLong`.
It seems that `batch_size_fail_threshold` is the only parameter of type `IntKibibytesBound` that suffers from this issue.
Now, I went ahead and did a further check for the other properties that are widen to long as it seems this bug was introduced when we removed the static methods from `DataStorageSpec`.
We will need to add `toBytesInLong` also for `IntMebibytesBound` to cover ```
getMinFreeSpacePerDriveInBytes() / min_free_space_per_drive,
compaction_large_partition_warning_threshold / getMinFreeSpacePerDriveInBytes(), getMaxHintsFileSize() / max_hints_file_size
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org