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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
