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/01/26 21:07:43 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request #875: Add possibility to copy SSTables in SSTableImporter instead of moving them
smiklosovic opened a new pull request #875:
URL: https://github.com/apache/cassandra/pull/875
----------------------------------------------------------------
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.
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] krummas commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r580868025
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java
##########
@@ -2276,8 +2276,24 @@ public static SSTableReader moveAndOpenSSTable(ColumnFamilyStore cfs, Descriptor
throw new RuntimeException(msg);
}
- logger.info("Renaming new SSTable {} to {}", oldDescriptor, newDescriptor);
- SSTableWriter.rename(oldDescriptor, newDescriptor, components);
+ if (copyData)
+ {
+ try
+ {
+ logger.info("Hardlinking new SSTable {} to {}", oldDescriptor, newDescriptor);
+ SSTableWriter.hardlink(oldDescriptor, newDescriptor, components);
+ }
+ catch (Throwable t)
Review comment:
we should not catch throwable here without rethrowing - we might hide OOM errors for example
##########
File path: src/java/org/apache/cassandra/io/util/FileUtils.java
##########
@@ -193,6 +171,58 @@ public static File createDeletableTempFile(String prefix, String suffix)
return f;
}
+ public static void createHardLink(String from, String to)
+ {
+ createHardLink(new File(from), new File(to));
+ }
+
+ public static void createHardLink(File from, File to)
+ {
+ if (to.exists())
+ throw new RuntimeException("Tried to create duplicate hard link to " + to);
+ if (!from.exists())
+ throw new RuntimeException("Tried to hard link to file that does not exist " + from);
+
+ try
+ {
+ Files.createLink(to.toPath(), from.toPath());
+ }
+ catch (IOException e)
+ {
+ throw new FSWriteError(e, to);
+ }
+ }
+
+ public static void createHardLinkWithConfirm(File from, File to)
+ {
+ try
+ {
+ createHardLink(from, to);
+ }
+ catch (Throwable t)
+ {
+ throw new RuntimeException(String.format("Unable to hardlink from %s to %s", from, to), t);
+ }
+ }
+
+ public static void createHardLinkWithConfirm(String from, String to)
+ {
+ createHardLinkWithConfirm(new File(from), new File(to));
+ }
+
+ public static void createHardLinkWithoutConfirm(String from, String to)
+ {
+ try
+ {
+ createHardLink(new File(from), new File(to));
+ }
+ catch (Throwable t)
Review comment:
same as above - don't catch throwable
----------------------------------------------------------------
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.
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 commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r580876931
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java
##########
@@ -2276,8 +2276,24 @@ public static SSTableReader moveAndOpenSSTable(ColumnFamilyStore cfs, Descriptor
throw new RuntimeException(msg);
}
- logger.info("Renaming new SSTable {} to {}", oldDescriptor, newDescriptor);
- SSTableWriter.rename(oldDescriptor, newDescriptor, components);
+ if (copyData)
+ {
+ try
+ {
+ logger.info("Hardlinking new SSTable {} to {}", oldDescriptor, newDescriptor);
+ SSTableWriter.hardlink(oldDescriptor, newDescriptor, components);
+ }
+ catch (Throwable t)
Review comment:
@krummas but that is the point, because in that catch block I am copying SSTables instead of hardlinking them. If I rethrow, how will I fall back to copy then?
----------------------------------------------------------------
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.
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] krummas commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r573474150
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java
##########
@@ -2276,8 +2276,13 @@ public static SSTableReader moveAndOpenSSTable(ColumnFamilyStore cfs, Descriptor
throw new RuntimeException(msg);
}
- logger.info("Renaming new SSTable {} to {}", oldDescriptor, newDescriptor);
- SSTableWriter.rename(oldDescriptor, newDescriptor, components);
+ if (copyData) {
+ logger.info("Copying new SSTable {} to {}", oldDescriptor, newDescriptor);
+ SSTableWriter.copy(oldDescriptor, newDescriptor, components);
Review comment:
We should probably try to hard link the file here? If it fails we can fall back to copy
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java
##########
@@ -2276,8 +2276,13 @@ public static SSTableReader moveAndOpenSSTable(ColumnFamilyStore cfs, Descriptor
throw new RuntimeException(msg);
}
- logger.info("Renaming new SSTable {} to {}", oldDescriptor, newDescriptor);
- SSTableWriter.rename(oldDescriptor, newDescriptor, components);
+ if (copyData) {
Review comment:
brace on newline (same for the whole patch)
##########
File path: src/java/org/apache/cassandra/io/util/FileUtils.java
##########
@@ -240,6 +240,35 @@ public static void deleteWithConfirmWithThrottle(File file, RateLimiter rateLimi
maybeFail(deleteWithConfirm(file, null, rateLimiter));
}
+ public static void copyWithOutConfirm(String from, String to)
+ {
+ try {
+ Files.copy(new File(from).toPath(), new File(to).toPath());
+ } catch (IOException e) {
+ if (logger.isTraceEnabled())
+ logger.trace("Could not copy file" + from + " to " + to, e);
+ }
+ }
+
+ public static void copyWithConfirm(String from, String to)
+ {
+ copyWithConfirm(new File(from), new File(to));
+ }
+
+ public static void copyWithConfirm(File from, File to)
+ {
+ assert from.exists();
+ if (logger.isTraceEnabled())
+ logger.trace("Copying {} to {}", from.getPath(), to.getPath());
+
+ try {
+ Files.copy(from.toPath(), to.toPath());
+ } catch (IOException e) {
Review comment:
we should not ignore `IOException` here
##########
File path: src/java/org/apache/cassandra/io/util/FileUtils.java
##########
@@ -240,6 +240,35 @@ public static void deleteWithConfirmWithThrottle(File file, RateLimiter rateLimi
maybeFail(deleteWithConfirm(file, null, rateLimiter));
}
+ public static void copyWithOutConfirm(String from, String to)
+ {
+ try {
+ Files.copy(new File(from).toPath(), new File(to).toPath());
Review comment:
nit: `Paths.get(from)`, `Paths.get(to)`
----------------------------------------------------------------
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.
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 commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r574457151
##########
File path: src/java/org/apache/cassandra/io/util/FileUtils.java
##########
@@ -66,7 +66,7 @@
import static org.apache.cassandra.utils.Throwables.maybeFail;
import static org.apache.cassandra.utils.Throwables.merge;
-public final class FileUtils
+public class FileUtils
Review comment:
It does not matter to have it final as it already has private constructor so it can not be extended anyway as extender can not call super and it just blocks mocking as it can not deal with finals.
----------------------------------------------------------------
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.
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] krummas commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r580882552
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java
##########
@@ -2276,8 +2276,24 @@ public static SSTableReader moveAndOpenSSTable(ColumnFamilyStore cfs, Descriptor
throw new RuntimeException(msg);
}
- logger.info("Renaming new SSTable {} to {}", oldDescriptor, newDescriptor);
- SSTableWriter.rename(oldDescriptor, newDescriptor, components);
+ if (copyData)
+ {
+ try
+ {
+ logger.info("Hardlinking new SSTable {} to {}", oldDescriptor, newDescriptor);
+ SSTableWriter.hardlink(oldDescriptor, newDescriptor, components);
+ }
+ catch (Throwable t)
Review comment:
catch `FSWriteError` or what is actually thrown by `SSTableWriter.hardlink`?
----------------------------------------------------------------
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.
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] michaelsembwever commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r585909254
##########
File path: src/java/org/apache/cassandra/tools/nodetool/Import.java
##########
@@ -75,6 +75,11 @@
description = "Run an extended verify, verifying all values in the new sstables")
private boolean extendedVerify = false;
+ @Option(title = "copy_data",
+ name = {"-p", "--copy-data"},
Review comment:
why `-p` ?
----------------------------------------------------------------
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.
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 commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r574456112
##########
File path: build.xml
##########
@@ -589,7 +589,8 @@
<dependency groupId="com.github.jbellis" artifactId="jamm" version="${jamm.version}"/>
<dependency groupId="org.yaml" artifactId="snakeyaml" version="1.26"/>
<dependency groupId="junit" artifactId="junit" version="4.12" />
- <dependency groupId="org.mockito" artifactId="mockito-core" version="3.2.4" />
+ <dependency groupId="org.mockito" artifactId="mockito-core" version="3.7.7" />
Review comment:
I had to increase the version as this version supports mocking of static methods (with use of Powermock I tried too but it was failing for various reasons), here we have static mocks available nicely. The first version supporting this is 3.4.0 but why not to bump it to the latest one if we are doing it anyway.
https://wttech.blog/blog/2020/mocking-static-methods-made-possible-in-mockito-3.4.0/
##########
File path: build.xml
##########
@@ -589,7 +589,8 @@
<dependency groupId="com.github.jbellis" artifactId="jamm" version="${jamm.version}"/>
<dependency groupId="org.yaml" artifactId="snakeyaml" version="1.26"/>
<dependency groupId="junit" artifactId="junit" version="4.12" />
- <dependency groupId="org.mockito" artifactId="mockito-core" version="3.2.4" />
+ <dependency groupId="org.mockito" artifactId="mockito-core" version="3.7.7" />
+ <dependency groupId="org.mockito" artifactId="mockito-inline" version="3.7.7" />
Review comment:
needed for statics
----------------------------------------------------------------
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.
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 #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #875:
URL: https://github.com/apache/cassandra/pull/875
--
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.
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 commented on a change in pull request #875: CASSANDRA-16407 - Add possibility to copy SSTables in SSTableImporter instead of moving them
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #875:
URL: https://github.com/apache/cassandra/pull/875#discussion_r576269549
##########
File path: src/java/org/apache/cassandra/io/util/FileUtils.java
##########
@@ -66,7 +66,7 @@
import static org.apache.cassandra.utils.Throwables.maybeFail;
import static org.apache.cassandra.utils.Throwables.merge;
-public final class FileUtils
+public class FileUtils
Review comment:
not relevant anymore
----------------------------------------------------------------
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.
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