You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "tiagomlalves (via GitHub)" <gi...@apache.org> on 2024/04/04 00:06:54 UTC
Re: [PR] CommitlogArchiver only has granularity to seconds for restore_point_in_time [cassandra]
tiagomlalves commented on code in PR #3215:
URL: https://github.com/apache/cassandra/pull/3215#discussion_r1550655265
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogMBean.java:
##########
@@ -41,7 +41,11 @@ public interface CommitLogMBean
/**
* Restore mutations created up to and including this timestamp in GMT
- * Format: yyyy:MM:dd HH:mm:ss (2012:04:31 20:43:12)
+ * There are only three different formats to express three time precisions:
+ * Seconds, Milliseconds, and Microseconds.
+ * Seconds format: yyyy:MM:dd HH:mm:ss (2012:04:31 20:43:12)
+ * Milliseconds format: yyyy:MM:dd HH:mm:ss[.[SSS]] (2012:04:31 20:43:12.633)
+ * Microseconds format: yyyy:MM:dd HH:mm:ss[.[SSSSSS]] (2012:04:31 20:43:12.633222)
Review Comment:
same comment as above
##########
conf/commitlog_archiving.properties:
##########
@@ -37,7 +37,11 @@ restore_command=
restore_directories=
# Restore mutations created up to and including this timestamp in GMT.
-# Format: yyyy:MM:dd HH:mm:ss (2012:04:31 20:43:12)
+# There are only three different formats to express three time precisions:
+# Seconds, Milliseconds, and Microseconds.
+# Seconds format: yyyy:MM:dd HH:mm:ss (2012:04:31 20:43:12)
+# Milliseconds format: yyyy:MM:dd HH:mm:ss[.[SSS]] (2012:04:31 20:43:12.633)
+# Microseconds format: yyyy:MM:dd HH:mm:ss[.[SSSSSS]] (2012:04:31 20:43:12.633222)
Review Comment:
I would propose removing the brackets which indicate optional/groups, and just specify:
```
# Milliseconds format: yyyy:MM:dd HH:mm:ss.SSS (2012:04:31 20:43:12.633)
# Microseconds format: yyyy:MM:dd HH:mm:ss.SSSSSS (2012:04:31 20:43:12.633222)
```
##########
test/unit/org/apache/cassandra/db/RecoveryManagerTest.java:
##########
@@ -250,28 +249,84 @@ public void testRecoverCounter() throws IOException
public void testRecoverPIT() throws Exception
{
CommitLog.instance.resetUnsafe(true);
+ long rpiTimeInit = CommitLog.instance.archiver.restorePointInTimeInMicros;
Review Comment:
Whenever I see `rpi` my brain parses it as Raspberry Pi 🤓 sorry!
What calling it `originalPIT` ? Later we'll be doing `.setRestorePointInTime(originalPIT);`
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogArchiver.java:
##########
@@ -103,62 +108,76 @@ public static CommitLogArchiver construct()
else
{
commitlog_commands.load(stream);
- String archiveCommand = commitlog_commands.getProperty("archive_command");
- String restoreCommand = commitlog_commands.getProperty("restore_command");
- String restoreDirectories = commitlog_commands.getProperty("restore_directories");
- if (restoreDirectories != null && !restoreDirectories.isEmpty())
+ return getArchiverFromProperty(commitlog_commands);
+ }
+ }
+ catch (IOException e)
+ {
+ throw new RuntimeException("Unable to load commitlog_archiving.properties", e);
+ }
+ }
+
+ public static CommitLogArchiver getArchiverFromProperty(Properties commitlogCommands)
+ {
+ assert !commitlogCommands.isEmpty();
+ String archiveCommand = commitlogCommands.getProperty("archive_command");
+ String restoreCommand = commitlogCommands.getProperty("restore_command");
+ String restoreDirectories = commitlogCommands.getProperty("restore_directories");
+ if (restoreDirectories != null && !restoreDirectories.isEmpty())
+ {
+ for (String dir : restoreDirectories.split(DELIMITER))
+ {
+ File directory = new File(dir);
+ if (!directory.exists())
{
- for (String dir : restoreDirectories.split(DELIMITER))
+ if (!directory.tryCreateDirectory())
{
- File directory = new File(dir);
- if (!directory.exists())
- {
- if (!directory.tryCreateDirectory())
- {
- throw new RuntimeException("Unable to create directory: " + dir);
- }
- }
+ throw new RuntimeException("Unable to create directory: " + dir);
}
}
- String targetTime = commitlog_commands.getProperty("restore_point_in_time");
- TimeUnit precision = TimeUnit.valueOf(commitlog_commands.getProperty("precision", "MICROSECONDS"));
- long restorePointInTime;
- try
- {
- restorePointInTime = Strings.isNullOrEmpty(targetTime) ? Long.MAX_VALUE : format.parse(targetTime).getTime();
- }
- catch (ParseException e)
- {
- throw new RuntimeException("Unable to parse restore target time", e);
- }
-
- String snapshotPosition = commitlog_commands.getProperty("snapshot_commitlog_position");
- CommitLogPosition snapshotCommitLogPosition;
- try
- {
+ }
+ }
+ String targetTime = commitlogCommands.getProperty("restore_point_in_time");
+ //todo remove this as this is not used
+ TimeUnit precision = TimeUnit.valueOf(commitlogCommands.getProperty("precision", "MICROSECONDS"));
Review Comment:
agree :)
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogArchiver.java:
##########
@@ -103,62 +108,76 @@ public static CommitLogArchiver construct()
else
{
commitlog_commands.load(stream);
- String archiveCommand = commitlog_commands.getProperty("archive_command");
- String restoreCommand = commitlog_commands.getProperty("restore_command");
- String restoreDirectories = commitlog_commands.getProperty("restore_directories");
- if (restoreDirectories != null && !restoreDirectories.isEmpty())
+ return getArchiverFromProperty(commitlog_commands);
+ }
+ }
+ catch (IOException e)
+ {
+ throw new RuntimeException("Unable to load commitlog_archiving.properties", e);
+ }
+ }
+
+ public static CommitLogArchiver getArchiverFromProperty(Properties commitlogCommands)
+ {
+ assert !commitlogCommands.isEmpty();
+ String archiveCommand = commitlogCommands.getProperty("archive_command");
+ String restoreCommand = commitlogCommands.getProperty("restore_command");
+ String restoreDirectories = commitlogCommands.getProperty("restore_directories");
+ if (restoreDirectories != null && !restoreDirectories.isEmpty())
+ {
+ for (String dir : restoreDirectories.split(DELIMITER))
+ {
+ File directory = new File(dir);
+ if (!directory.exists())
{
- for (String dir : restoreDirectories.split(DELIMITER))
+ if (!directory.tryCreateDirectory())
{
- File directory = new File(dir);
- if (!directory.exists())
- {
- if (!directory.tryCreateDirectory())
- {
- throw new RuntimeException("Unable to create directory: " + dir);
- }
- }
+ throw new RuntimeException("Unable to create directory: " + dir);
}
}
- String targetTime = commitlog_commands.getProperty("restore_point_in_time");
- TimeUnit precision = TimeUnit.valueOf(commitlog_commands.getProperty("precision", "MICROSECONDS"));
- long restorePointInTime;
- try
- {
- restorePointInTime = Strings.isNullOrEmpty(targetTime) ? Long.MAX_VALUE : format.parse(targetTime).getTime();
- }
- catch (ParseException e)
- {
- throw new RuntimeException("Unable to parse restore target time", e);
- }
-
- String snapshotPosition = commitlog_commands.getProperty("snapshot_commitlog_position");
- CommitLogPosition snapshotCommitLogPosition;
- try
- {
+ }
+ }
+ String targetTime = commitlogCommands.getProperty("restore_point_in_time");
+ //todo remove this as this is not used
+ TimeUnit precision = TimeUnit.valueOf(commitlogCommands.getProperty("precision", "MICROSECONDS"));
+ long restorePointInTime;
+ try
+ {
+ if (Strings.isNullOrEmpty(targetTime))
+ {
+ restorePointInTime = Long.MAX_VALUE;
+ }
+ else
+ {
+ // get restorePointInTime microlevel by default as c* use this level ts.
+ restorePointInTime = getMicroSeconds(targetTime);
+ }
+ }
+ catch (DateTimeParseException | ConfigurationException e)
+ {
+ throw new RuntimeException("Unable to parse restore target time", e);
+ }
Review Comment:
Where does `ConfigurationException` comes from?
Here, I would rewrite this as:
```
long restorePointInTime = Long.MAX_VALUE;
try
{
if (!Strings.isNullOrEmpty(targetTime))
{
// get restorePointInTime microlevel by default as c* use this level ts.
restorePointInTime = getMicroSeconds(targetTime);
}
}
```
or just (which is also used below):
```
long restorePointInTime;
try
{
restorePointInTime = Strings.isNullOrEmpty(targetTime)
? Long.MAX_VALUE
: getMicroSeconds(targetTime);
}
```
to reduce the nesting.
##########
test/unit/org/apache/cassandra/cql3/validation/operations/DropRecreateAndRestoreTest.java:
##########
@@ -71,13 +71,13 @@ public void testCreateWithIdRestore() throws Throwable
FileUtils.renameWithConfirm(new File(logPath, segment + ".save"), new File(logPath, segment));
try
{
- // Restore to point in time.
- CommitLog.instance.archiver.restorePointInTime = time;
+ // Restore to point in time, the rpi time is micro level
Review Comment:
what about?
```
// Restore to point in time (microseconds granularity).
```
--
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