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 2021/12/05 18:16:51 UTC
[GitHub] [cassandra] subkanthi opened a new pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
subkanthi opened a new pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354
[16436] - Added startup check for read_ahead_kb setting.
--
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] asfgit closed pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r774585032
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Added logic, thanks guys.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r765397926
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Just to clarify the steps.
1) Iterate through the data directories.
2) Get the volume using Files.getFileStore() for the directory, Example it returns /dev/sda2 for /home/$user/cassandra/data
3) Read the sys path ignoring the numbers from the volume returned in step 2 - /sys/block/**sda**/queue/read_ahead_kb
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777235190
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
Review comment:
sure, refactored
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r774586101
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
Review comment:
Changed to 128, thanks
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777225477
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -243,8 +247,8 @@ private void checkOutOfMemoryHandling()
{
if (!jvmOptionsContainsOneOf("-XX:OnOutOfMemoryError="))
logger.warn("The JVM is not configured to stop on OutOfMemoryError which can cause data corruption."
- + " Either upgrade your JRE to a version greater or equal to 8u92 and use -XX:+ExitOnOutOfMemoryError/-XX:+CrashOnOutOfMemoryError"
- + " or use -XX:OnOutOfMemoryError=\"<cmd args>;<cmd args>\" on your current JRE.");
+ + " Either upgrade your JRE to a version greater or equal to 8u92 and use -XX:+ExitOnOutOfMemoryError/-XX:+CrashOnOutOfMemoryError"
Review comment:
Removed, thanks.
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -498,23 +573,45 @@ public void execute() throws StartupException
throw new StartupException(StartupException.ERR_WRONG_CONFIG, errMsg.get());
};
+ @VisibleForTesting
+ public static String getReadAheadKBPath(String blockDirectoryPath)
+ {
+ String readAheadKBPath = null;
+
+ final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/%s/queue/read_ahead_kb";
+ try
+ {
+ String deviceName = blockDirectoryPath.split("/")[2].replaceAll("[0-9]*$", "");
+
+ if (StringUtils.isNotEmpty(deviceName))
+ {
+ readAheadKBPath = String.format(READ_AHEAD_KB_SETTING_PATH, deviceName);
+ }
+ }
+ catch (Exception e)
+ {
+ logger.error("Error retrieving device path for {}.", blockDirectoryPath);
+ }
+
+ return readAheadKBPath;
+ }
+
@VisibleForTesting
static Optional<String> checkLegacyAuthTablesMessage()
{
List<String> existing = new ArrayList<>(SchemaConstants.LEGACY_AUTH_TABLES).stream().filter((legacyAuthTable) ->
- {
- UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
- SchemaConstants.SCHEMA_KEYSPACE_NAME,
- "tables",
- SchemaConstants.AUTH_KEYSPACE_NAME,
- legacyAuthTable));
- return result != null && !result.isEmpty();
- }).collect(Collectors.toList());
+ {
+ UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
+ SchemaConstants.SCHEMA_KEYSPACE_NAME, "tables", SchemaConstants.AUTH_KEYSPACE_NAME, legacyAuthTable));
+ return result != null && !result.isEmpty();
+ }).collect(Collectors.toList());
if (!existing.isEmpty())
return Optional.of(String.format("Legacy auth tables %s in keyspace %s still exist and have not been properly migrated.",
- Joiner.on(", ").join(existing), SchemaConstants.AUTH_KEYSPACE_NAME));
+ Joiner.on(", ").join(existing), SchemaConstants.AUTH_KEYSPACE_NAME));
Review comment:
Removed.
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -498,23 +573,45 @@ public void execute() throws StartupException
throw new StartupException(StartupException.ERR_WRONG_CONFIG, errMsg.get());
};
+ @VisibleForTesting
+ public static String getReadAheadKBPath(String blockDirectoryPath)
+ {
+ String readAheadKBPath = null;
+
+ final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/%s/queue/read_ahead_kb";
+ try
+ {
+ String deviceName = blockDirectoryPath.split("/")[2].replaceAll("[0-9]*$", "");
+
+ if (StringUtils.isNotEmpty(deviceName))
+ {
+ readAheadKBPath = String.format(READ_AHEAD_KB_SETTING_PATH, deviceName);
+ }
+ }
+ catch (Exception e)
+ {
+ logger.error("Error retrieving device path for {}.", blockDirectoryPath);
+ }
+
+ return readAheadKBPath;
+ }
+
@VisibleForTesting
static Optional<String> checkLegacyAuthTablesMessage()
{
List<String> existing = new ArrayList<>(SchemaConstants.LEGACY_AUTH_TABLES).stream().filter((legacyAuthTable) ->
- {
- UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
- SchemaConstants.SCHEMA_KEYSPACE_NAME,
- "tables",
- SchemaConstants.AUTH_KEYSPACE_NAME,
- legacyAuthTable));
- return result != null && !result.isEmpty();
- }).collect(Collectors.toList());
+ {
Review comment:
Removed.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777225574
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,30 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ // This test just validates if the verify function
+ // doesn't throw any exceptions
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
+ }
+
+ @Test
+ public void getReadAheadKBPath()
Review comment:
Changed, I wasnt really sure of the convention because other test cases didnt have the test prefix.
failStartupIfInvalidSSTablesFound
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r765397926
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Just to clarify the steps.
1) Iterate through the data directories.
2) Get the volume using Files.getFileStore(data_directory) for the directory, Example it returns /dev/sda2 for /home/$user/cassandra/data
3) Read the sys path ignoring the numbers from the volume returned in step 2 - /sys/block/**sda**/queue/read_ahead_kb
--
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] dpoluyanov commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
dpoluyanov commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r762609774
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Probably should be checked not only on `sda` device, but on all data directories (not sure that it's reasonable to check `read_ahead_kb` for commit log directory)
It's common to set more than one directory in `data_file_directories` property in `cassandra.yaml`
```
data_file_directories:
- /srv/disk01
- /srv/disk02
```
`lsblk` could be used to find mount point on common hardware (and to match device name with directory)
```
$ lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda 8:0 0 1.5T 0 disk /srv/disk01
sdb 8:16 0 1.5T 0 disk
`-md0 9:0 0 5.8T 0 raid0
`-md0p1 259:2 0 5.8T 0 part /srv/disk02
...
```
but pay special attention for case with software raid, where mount point could be on raid partition like on example above
software raid also could have it's own `read_ahead_kb` and makes sense to check it too.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777225596
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,30 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ // This test just validates if the verify function
+ // doesn't throw any exceptions
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
+ }
+
+ @Test
+ public void getReadAheadKBPath()
+ {
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+
+ String sdaDirectory = startupChecks.getReadAheadKBPath("/dev/sda12");
Review comment:
Changed. thanks.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777237401
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
Review comment:
@pauloricardomg can you please expand on this 'Also perhaps add a simple test with a temporary directory' , is the test for the getBlockDevices which will just return the Map?
--
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] pauloricardomg commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
pauloricardomg commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r763358813
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
Review comment:
can we rename this to `MAX_RECOMMENDED_READ_AHEAD_KB_SETTING`, and set the max recommended to 128 (according to [this post](https://tobert.github.io/pages/als-cassandra-21-tuning-guide.html)) in case people use a slightly higher value for this ? We can keep the suggested value in the startup check error message to 8.
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,13 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
Review comment:
Yeah this is ok, it's just testing the check will not throw any exception. Maybe add a little comment about this?
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ final Path path = Paths.get(READ_AHEAD_KB_SETTING_PATH);
+ try (final BufferedReader bufferedReader = Files.newBufferedReader(path))
+ {
+ final String data = bufferedReader.readLine();
+ if (data != null)
+ {
+ try
+ {
+ Integer readAheadKbSetting = Integer.parseInt(data);
+
+ if (readAheadKbSetting > EXPECTED_READ_AHEAD_KB_SETTING)
+ {
+ logger.warn("High Read Ahead Kb setting: It is Recommended to set this value to 8KB " +
Review comment:
Maybe prefix the message with: `Detected high 'read_ahead_kb' setting for device 'sda' of data directory '/srv/disk01'. It is Recommended to set this value to 8KB....`
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
yeah ideally we'll want to go through all data dirs and dynamically find the underlying devices to be able to lookup the `read_ahead_kb` setting. I'd like to avoid using `lsblk` and use something native to java for this check if possible (ie. [Files.getFileStore](https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#getFileStore(java.nio.file.Path)). Ideally we would want to do a manual test with this with multiple data dirs and a raid system like in your example to make sure the test is working. Do you have acess to a box where you can test this?
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777237401
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
Review comment:
@pauloricardomg can you please expand on this '`Also perhaps add a simple test with a temporary directory`' , is the test for the getBlockDevices which will just return the Map?
--
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] pauloricardomg commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
pauloricardomg commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777250351
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
Review comment:
the idea is to verify that running `getBlockDevices(new String[]{"/tmp/mytmpdir"})` will correctly return the block device associated with this temporary directory.
--
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] dpoluyanov commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
dpoluyanov commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r767247690
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
yep, looks good, with some clarification: after step 2 collect all paths (without numbers) into `Set` before going to step 3, in order to avoid duplicate messages when two data directories points onto the same underlying device
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r765397926
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Just to clarify the steps.
1) Iterate through the data directories.
2) Get the volume using Files.getFileStore() for the directory, Example it returns /dev/sda2 for /home/$user/cassandra/data
3) Read the path ignoring the numbers from the volume returned in step 2 - /sys/block/**sda**/queue/read_ahead_kb
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r774584867
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,13 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
Review comment:
Added.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777235190
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
Review comment:
Actually not sure what is happening, changing it to below, changes the `readAheadKbSetting` value to this number `825374730` and not `128`.
```
int readAheadKbSetting = Ints.fromByteArray(Files.readAllBytes(readAheadKBPath));
```
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r765397926
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Just to clarify the steps.
1) Iterate through the data directories.
2) Get the /dev directory location using Files.getFileStore(data_directory) for the directory, Example it returns **/dev/sda2** for /home/$user/cassandra/data
3) Read the sys path ignoring the numbers from the volume returned in step 2 - /sys/block/**sda**/queue/read_ahead_kb
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777235548
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
+ final String data = bufferedReader.readLine();
+ if (data != null)
+ {
+ try
+ {
+ Integer readAheadKbSetting = Integer.parseInt(data);
+
+ if (readAheadKbSetting > MAX_RECOMMENDED_READ_AHEAD_KB_SETTING)
+ {
+ logger.warn("Detected high 'read_ahead_kb' setting for device {} " +
+ "of data directory.{} It is Recommended to set this value to 8KB " +
Review comment:
Sorry, whats wrong with this message
```
WARN [main] 2022-01-02 12:26:51,241 StartupChecks.java:356 - Detected high 'read_ahead_kb' setting for device /dev/sda2 of data directory /home/parallels/Documents/GITHUB/cassandra/data/data It is Recommended to set this value to 8KB or lower as a higher value can cause high IO usage and cache churn on read-intensive workloads.
```
--
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] pauloricardomg commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
pauloricardomg commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r767741885
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
sounds good 👍
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r765397926
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
Review comment:
Just to clarify the steps.
1) Iterate through the data directories.
2) Get the volume using Files.getFileStore() for the directory, Example it returns /dev/sda2 for /home/<user>/cassandra/data
3) Read the path ignoring the numbers from the volume returned in step 2 - /sys/block/**sda**/queue/read_ahead_kb
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r774595790
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +286,49 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long EXPECTED_READ_AHEAD_KB_SETTING = 8;
+ private static final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/sda/queue/read_ahead_kb";
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ final Path path = Paths.get(READ_AHEAD_KB_SETTING_PATH);
+ try (final BufferedReader bufferedReader = Files.newBufferedReader(path))
+ {
+ final String data = bufferedReader.readLine();
+ if (data != null)
+ {
+ try
+ {
+ Integer readAheadKbSetting = Integer.parseInt(data);
+
+ if (readAheadKbSetting > EXPECTED_READ_AHEAD_KB_SETTING)
+ {
+ logger.warn("High Read Ahead Kb setting: It is Recommended to set this value to 8KB " +
Review comment:
Changed, thanks
--
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] subkanthi commented on pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#issuecomment-1000334002
@pauloricardomg , @dpoluyanov I have made the changes, please check whenever u get a chance.
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r762597216
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,13 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
Review comment:
Not sure if there should be some sort of assert, because this doesnt test much.
--
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] pauloricardomg commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
pauloricardomg commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777216698
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -498,23 +573,45 @@ public void execute() throws StartupException
throw new StartupException(StartupException.ERR_WRONG_CONFIG, errMsg.get());
};
+ @VisibleForTesting
+ public static String getReadAheadKBPath(String blockDirectoryPath)
+ {
+ String readAheadKBPath = null;
+
+ final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/%s/queue/read_ahead_kb";
+ try
+ {
+ String deviceName = blockDirectoryPath.split("/")[2].replaceAll("[0-9]*$", "");
+
+ if (StringUtils.isNotEmpty(deviceName))
+ {
+ readAheadKBPath = String.format(READ_AHEAD_KB_SETTING_PATH, deviceName);
+ }
+ }
+ catch (Exception e)
+ {
+ logger.error("Error retrieving device path for {}.", blockDirectoryPath);
+ }
+
+ return readAheadKBPath;
+ }
+
@VisibleForTesting
static Optional<String> checkLegacyAuthTablesMessage()
{
List<String> existing = new ArrayList<>(SchemaConstants.LEGACY_AUTH_TABLES).stream().filter((legacyAuthTable) ->
- {
- UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
- SchemaConstants.SCHEMA_KEYSPACE_NAME,
- "tables",
- SchemaConstants.AUTH_KEYSPACE_NAME,
- legacyAuthTable));
- return result != null && !result.isEmpty();
- }).collect(Collectors.toList());
+ {
+ UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
+ SchemaConstants.SCHEMA_KEYSPACE_NAME, "tables", SchemaConstants.AUTH_KEYSPACE_NAME, legacyAuthTable));
+ return result != null && !result.isEmpty();
+ }).collect(Collectors.toList());
if (!existing.isEmpty())
return Optional.of(String.format("Legacy auth tables %s in keyspace %s still exist and have not been properly migrated.",
- Joiner.on(", ").join(existing), SchemaConstants.AUTH_KEYSPACE_NAME));
+ Joiner.on(", ").join(existing), SchemaConstants.AUTH_KEYSPACE_NAME));
Review comment:
Please remove unrelated formatting change.
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,30 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ // This test just validates if the verify function
+ // doesn't throw any exceptions
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
+ }
+
+ @Test
+ public void getReadAheadKBPath()
+ {
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+
+ String sdaDirectory = startupChecks.getReadAheadKBPath("/dev/sda12");
Review comment:
use `StartupChecks.getReadAheadKBPath` to make explicit this is an static method.
##########
File path: test/unit/org/apache/cassandra/service/StartupChecksTest.java
##########
@@ -103,6 +103,30 @@ public void compatibilityCheckIgnoresNonDbFiles() throws Exception
startupChecks.verify();
}
+ @Test
+ public void checkReadAheadKbSettingCheck() throws Exception
+ {
+ // This test just validates if the verify function
+ // doesn't throw any exceptions
+ startupChecks = startupChecks.withTest(StartupChecks.checkReadAheadKbSetting);
+ startupChecks.verify();
+ }
+
+ @Test
+ public void getReadAheadKBPath()
Review comment:
call this `testGetReadAheadKBPath`
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -498,23 +573,45 @@ public void execute() throws StartupException
throw new StartupException(StartupException.ERR_WRONG_CONFIG, errMsg.get());
};
+ @VisibleForTesting
+ public static String getReadAheadKBPath(String blockDirectoryPath)
+ {
+ String readAheadKBPath = null;
+
+ final String READ_AHEAD_KB_SETTING_PATH = "/sys/block/%s/queue/read_ahead_kb";
+ try
+ {
+ String deviceName = blockDirectoryPath.split("/")[2].replaceAll("[0-9]*$", "");
+
+ if (StringUtils.isNotEmpty(deviceName))
+ {
+ readAheadKBPath = String.format(READ_AHEAD_KB_SETTING_PATH, deviceName);
+ }
+ }
+ catch (Exception e)
+ {
+ logger.error("Error retrieving device path for {}.", blockDirectoryPath);
+ }
+
+ return readAheadKBPath;
+ }
+
@VisibleForTesting
static Optional<String> checkLegacyAuthTablesMessage()
{
List<String> existing = new ArrayList<>(SchemaConstants.LEGACY_AUTH_TABLES).stream().filter((legacyAuthTable) ->
- {
- UntypedResultSet result = QueryProcessor.executeOnceInternal(String.format("SELECT table_name FROM %s.%s WHERE keyspace_name='%s' AND table_name='%s'",
- SchemaConstants.SCHEMA_KEYSPACE_NAME,
- "tables",
- SchemaConstants.AUTH_KEYSPACE_NAME,
- legacyAuthTable));
- return result != null && !result.isEmpty();
- }).collect(Collectors.toList());
+ {
Review comment:
Please remove unrelated formatting change.
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
Review comment:
Perhaps we can have a slightly more succint check:
```
Path readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
if (readAheadKBPath != null && Files.exists(readAheadKBPath))
{
int readAheadKbSetting = Ints.fromBytesArray(Files.readAllBytes(readAheadKBPath));
// perform check
}
```
wdyt?
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -243,8 +247,8 @@ private void checkOutOfMemoryHandling()
{
if (!jvmOptionsContainsOneOf("-XX:OnOutOfMemoryError="))
logger.warn("The JVM is not configured to stop on OutOfMemoryError which can cause data corruption."
- + " Either upgrade your JRE to a version greater or equal to 8u92 and use -XX:+ExitOnOutOfMemoryError/-XX:+CrashOnOutOfMemoryError"
- + " or use -XX:OnOutOfMemoryError=\"<cmd args>;<cmd args>\" on your current JRE.");
+ + " Either upgrade your JRE to a version greater or equal to 8u92 and use -XX:+ExitOnOutOfMemoryError/-XX:+CrashOnOutOfMemoryError"
Review comment:
Please remove unrelated formatting change.
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
+ final String data = bufferedReader.readLine();
+ if (data != null)
+ {
+ try
+ {
+ Integer readAheadKbSetting = Integer.parseInt(data);
+
+ if (readAheadKbSetting > MAX_RECOMMENDED_READ_AHEAD_KB_SETTING)
+ {
+ logger.warn("Detected high 'read_ahead_kb' setting for device {} " +
+ "of data directory.{} It is Recommended to set this value to 8KB " +
Review comment:
Please fix message
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
Review comment:
To improve readability and testability, maybe extract this to a different method?
Something like:
```java
Map<String, String> blockDevices = getBlockDevices(DatabaseDescriptor.getRawConfig().data_file_directories);
```
Also perhaps add a simple test with a temporary directory (see: https://www.javacodegeeks.com/2021/03/creating-temporary-files-with-junit-5.html).
--
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] subkanthi commented on a change in pull request #1354: [16436] - Added startup check for read_ahead_kb setting.
Posted by GitBox <gi...@apache.org>.
subkanthi commented on a change in pull request #1354:
URL: https://github.com/apache/cassandra/pull/1354#discussion_r777235190
##########
File path: src/java/org/apache/cassandra/service/StartupChecks.java
##########
@@ -285,6 +289,77 @@ public void execute()
}
};
+ public static final StartupCheck checkReadAheadKbSetting = new StartupCheck()
+ {
+ // This value is in KB.
+ private static final long MAX_RECOMMENDED_READ_AHEAD_KB_SETTING = 128;
+
+ @Override
+ public void execute() throws StartupException
+ {
+ if (!FBUtilities.isLinux)
+ return;
+
+ String[] dataDirectories = DatabaseDescriptor.getRawConfig().data_file_directories;
+ Map<String, String> blockDevices = new HashMap<String, String>();
+ for (String dataDirectory : dataDirectories)
+ {
+ try
+ {
+ Path p = Path.of(dataDirectory);
+ FileStore fs = Files.getFileStore(p);
+
+ String blockDirectory = fs.name();
+ if(StringUtils.isNotEmpty(blockDirectory))
+ {
+ blockDevices.put(blockDirectory, dataDirectory);
+ }
+ }
+ catch (IOException e)
+ {
+ logger.warn("IO exception while reading file {}.", dataDirectory, e);
+ }
+ }
+ for (Map.Entry<String, String> entry: blockDevices.entrySet())
+ {
+ String blockDeviceDirectory = entry.getKey();
+ String dataDirectory = entry.getValue();
+ try
+ {
+
+ String readAheadKBPath = StartupChecks.getReadAheadKBPath(blockDeviceDirectory);
+
+ final BufferedReader bufferedReader = Files.newBufferedReader(Path.of(readAheadKBPath));
Review comment:
Actually not sure what is happening, changing it to below, changes the value to this number `825374730` and not `128`.
```
int readAheadKbSetting = Ints.fromByteArray(Files.readAllBytes(readAheadKBPath));
```
--
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