Copilot commented on code in PR #8761:
URL: https://github.com/apache/ozone/pull/8761#discussion_r2217959023
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotUtils.java:
##########
@@ -124,20 +129,28 @@ public static Path createHardLinkList(int truncateLength,
* Create hard links listed in OM_HARDLINK_FILE.
*
* @param dbPath Path to db to have links created.
+ * @param deleteSourceFiles - Whether to delete the source files after
creating the links.
*/
- public static void createHardLinks(Path dbPath) throws IOException {
+ public static void createHardLinks(Path dbPath, boolean deleteSourceFiles)
throws IOException {
File hardLinkFile =
new File(dbPath.toString(), OmSnapshotManager.OM_HARDLINK_FILE);
+ List<Path> filesToDelete = new ArrayList<>();
if (hardLinkFile.exists()) {
// Read file.
try (Stream<String> s = Files.lines(hardLinkFile.toPath())) {
List<String> lines = s.collect(Collectors.toList());
// Create a link for each line.
for (String l : lines) {
- String from = l.split("\t")[1];
- String to = l.split("\t")[0];
+ String[] parts = l.split("\t");
+ if (parts.length != 2) {
+ LOG.warn("Skipping malformed line in hardlink file: {}", l);
+ return;
Review Comment:
Using 'return' instead of 'continue' will exit the entire method when
encountering a malformed line, preventing processing of subsequent valid lines.
This should be 'continue' to skip only the malformed line and process remaining
lines.
```suggestion
continue;
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HAUtils.java:
##########
@@ -319,25 +318,21 @@ public static File getMetaDir(DBDefinition definition,
}
/**
- * Scan the DB dir and return the existing SST files,
- * including omSnapshot sst files.
- * SSTs could be used for avoiding repeated download.
+ * Scan the DB dir and return the existing files,
+ * including omSnapshot files.
*
* @param db the file representing the DB to be scanned
- * @return the list of SST file name. If db not exist, will return empty list
+ * @return the list of file names. If db not exist, will return empty list
*/
- public static List<String> getExistingSstFiles(File db) throws IOException {
+ public static List<String> getExistingFiles(File db) throws IOException {
List<String> sstList = new ArrayList<>();
if (!db.exists()) {
return sstList;
}
-
- int truncateLength = db.toString().length() + 1;
// Walk the db dir and get all sst files including omSnapshot files.
try (Stream<Path> files = Files.walk(db.toPath())) {
- sstList =
- files.filter(path -> path.toString().endsWith(ROCKSDB_SST_SUFFIX)).
- map(p -> p.toString().substring(truncateLength)).
+ sstList = files.filter(p -> p.toFile().isFile())
+ .map(p -> p.getFileName().toString()).
collect(Collectors.toList());
if (LOG.isDebugEnabled()) {
LOG.debug("Scanned SST files {} in {}.", sstList,
db.getAbsolutePath());
Review Comment:
The log message still refers to 'SST files' but the method now scans all
files. The message should be updated to 'Scanned files {} in {}.' to accurately
reflect the current behavior.
```suggestion
LOG.debug("Scanned files {} in {}.", sstList, db.getAbsolutePath());
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -1199,10 +1199,7 @@ private Set<String> getSstFilenames(File tarball)
new TarArchiveInputStream(Files.newInputStream(tarball.toPath()))) {
TarArchiveEntry entry;
while ((entry = tarInput.getNextTarEntry()) != null) {
- String name = entry.getName();
- if (name.toLowerCase().endsWith(".sst")) {
- sstFilenames.add(entry.getName());
- }
+ sstFilenames.add(entry.getName());
Review Comment:
The condition checking for SST files was removed, so this method will now
add all tar entries to sstFilenames regardless of file type. The method name
'getSstFilenames' suggests it should only return SST files, but it now returns
all files.
```suggestion
if (entry.getName().toLowerCase().endsWith(".sst")) {
sstFilenames.add(entry.getName());
}
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -1173,8 +1174,7 @@ public void pause() throws IOException {
// Get Size of sstfiles in tarball.
private long getSizeOfSstFiles(File tarball) throws IOException {
FileUtil.unTar(tarball, tempDir.toFile());
- List<Path> sstPaths = Files.walk(tempDir).filter(
- path -> path.toString().endsWith(".sst")).
+ List<Path> sstPaths = Files.walk(tempDir).
collect(Collectors.toList());
Review Comment:
The filter condition for SST files was removed, but the variable name
'sstPaths' and subsequent logic still assume SST files. This will now include
all files, not just SST files, which may cause incorrect size calculations and
behavior.
```suggestion
List<Path> sstPaths = Files.walk(tempDir)
.filter(path -> Files.isRegularFile(path) &&
path.toString().endsWith(".sst"))
.collect(Collectors.toList());
```
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
