You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/12/23 09:24:11 UTC
[GitHub] [commons-compress] PostelnicuGeorge opened a new pull request #236: COMPRESS-602 - migrate zip package to nio path
PostelnicuGeorge opened a new pull request #236:
URL: https://github.com/apache/commons-compress/pull/236
Implementation of: https://issues.apache.org/jira/browse/COMPRESS-602
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775067730
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -23,21 +23,26 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
+import java.nio.file.Path;
/**
- * ScatterGatherBackingStore that is backed by a file.
+ * ScatterGatherBackingStore that is backed by a file or path.
*
- * @since 1.10
+ * @since 1.22
*/
public class FileBasedScatterGatherBackingStore implements ScatterGatherBackingStore {
- private final File target;
+ private final Path target;
private final OutputStream os;
private boolean closed;
public FileBasedScatterGatherBackingStore(final File target) throws FileNotFoundException {
+ this(target.toPath());
+ }
+
+ public FileBasedScatterGatherBackingStore(final Path target) throws FileNotFoundException {
Review comment:
This new eleemnt needs a Javadoc comment and since tag.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774558206
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ParallelScatterZipCreator.java
##########
@@ -17,13 +17,11 @@
*/
package org.apache.commons.compress.archivers.zip;
-import org.apache.commons.compress.parallel.FileBasedScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.InputStreamSupplier;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStoreSupplier;
+import org.apache.commons.compress.parallel.*;
Review comment:
We do not use wildcard imports. Please specify the packages you want to import.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774501046
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStream.java
##########
@@ -198,7 +210,19 @@ public static ScatterZipOutputStream fileBased(final File file) throws FileNotFo
* @throws FileNotFoundException if the file cannot be found
*/
public static ScatterZipOutputStream fileBased(final File file, final int compressionLevel) throws FileNotFoundException {
- final ScatterGatherBackingStore bs = new FileBasedScatterGatherBackingStore(file);
+ return pathBased(file.toPath(), compressionLevel);
+ }
+
+ /**
+ * Create a {@link ScatterZipOutputStream} that is backed by a file
+ *
+ * @param path The path to offload compressed data into.
+ * @param compressionLevel The compression level to use, @see #Deflater
+ * @return A ScatterZipOutputStream that is ready for use.
+ * @throws FileNotFoundException if the path cannot be found
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -518,6 +543,11 @@ public void setUseZip64(final Zip64Mode mode) {
zip64Mode = mode;
}
+ @Override
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -210,7 +222,21 @@ public ZipFile(final String name, final String encoding) throws IOException {
* @throws IOException if an error occurs while reading the file.
*/
public ZipFile(final File f, final String encoding) throws IOException {
- this(f, encoding, true);
+ this(f.toPath(), encoding, true);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names and scanning for unicode extra fields.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
+ *
+ * @throws IOException if an error occurs while reading the file.
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -227,7 +253,24 @@ public ZipFile(final File f, final String encoding) throws IOException {
*/
public ZipFile(final File f, final String encoding, final boolean useUnicodeExtraFields)
throws IOException {
- this(f, encoding, useUnicodeExtraFields, false);
+ this(f.toPath(), encoding, useUnicodeExtraFields, false);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
+ * @param useUnicodeExtraFields whether to use InfoZIP Unicode
+ * Extra Fields (if present) to set the file names.
Review comment:
Ditto
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Hm, why? How is this part of the NIO change? Hard to te with so many changes...
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -262,6 +305,39 @@ public ZipFile(final File f, final String encoding, final boolean useUnicodeExtr
f.getAbsolutePath(), encoding, useUnicodeExtraFields, true, ignoreLocalFileHeader);
}
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names.
+ *
+ *
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -210,7 +222,21 @@ public ZipFile(final String name, final String encoding) throws IOException {
* @throws IOException if an error occurs while reading the file.
*/
public ZipFile(final File f, final String encoding) throws IOException {
- this(f, encoding, true);
+ this(f.toPath(), encoding, true);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names and scanning for unicode extra fields.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
Review comment:
No need for extra whitespace.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -174,6 +175,17 @@ public ZipFile(final File f) throws IOException {
this(f, ZipEncodingHelper.UTF8);
}
+ /**
+ * Opens the given path for reading, assuming "UTF8" for file names.
+ *
+ * @param path path to the archive.
+ *
+ * @throws IOException if an error occurs while reading the file.
+ */
+ public ZipFile(final Path path) throws IOException {
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -57,19 +58,31 @@
* @param zipFile the zip file to write to
* @param splitSize the split size
*/
- public ZipSplitOutputStream(final File zipFile, final long splitSize) throws IllegalArgumentException, IOException {
+ public ZipSplitOutputStream(final Path zipFile, final long splitSize) throws IllegalArgumentException, IOException {
if (splitSize < ZIP_SEGMENT_MIN_SIZE || splitSize > ZIP_SEGMENT_MAX_SIZE) {
throw new IllegalArgumentException("zip split segment size should between 64K and 4,294,967,295");
}
this.zipFile = zipFile;
this.splitSize = splitSize;
- this.outputStream = Files.newOutputStream(zipFile.toPath());
+ this.outputStream = Files.newOutputStream(zipFile);
// write the zip split signature 0x08074B50 to the zip file
writeZipSplitSignature();
}
+ /**
+ * Create a split zip. If the zip file is smaller than the split size,
+ * then there will only be one split zip, and its suffix is .zip,
+ * otherwise the split segments should be like .z01, .z02, ... .z(N-1), .zip
+ *
+ * @param zipFile the zip file to write to
+ * @param splitSize the split size
+ */
Review comment:
Missing since tag on new or protected method.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -365,8 +365,33 @@ public ZipArchiveOutputStream(final Path file, final OpenOption... options) thro
* @since 1.20
*/
public ZipArchiveOutputStream(final File file, final long zipSplitSize) throws IOException {
+ this(file.toPath(), zipSplitSize);
+ }
+
+ /**
+ * Creates a split ZIP Archive.
+ *
+ * <p>The files making up the archive will use Z01, Z02,
+ * ... extensions and the last part of it will be the given {@code
+ * file}.</p>
+ *
+ * <p>Even though the stream writes to a file this stream will
+ * behave as if no random access was possible. This means the
+ * sizes of stored entries need to be known before the actual
+ * entry data is written.</p>
+ *
+ * @param path the file that will become the last part of the split archive
+ * @param zipSplitSize maximum size of a single part of the split
+ * archive created by this stream. Must be between 64kB and about
+ * 4GB.
+ *
Review comment:
No need for extra whitespace.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public ZipSplitReadOnlySeekableByteChannel(final List<SeekableByteChannel> channels)
- throws IOException {
+ throws IOException {
Review comment:
Don't change formatting.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -47,7 +50,7 @@
public class ZipSplitReadOnlySeekableByteChannel extends MultiReadOnlySeekableByteChannel {
private static final int ZIP_SPLIT_SIGNATURE_LENGTH = 4;
private final ByteBuffer zipSplitSignatureByteBuffer =
- ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
+ ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
Review comment:
Please do not change the formatting, it makes reviewing harder and take more time.
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,19 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ public static String getExtensionFrom(final Path path) {
Review comment:
Make this private or package private. The fewer public or protected method the better as they will have to be maintained "forever". Also needs a comment.
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir, "testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
Review comment:
-1: Local var names never start with a CAPITAL letter.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -108,10 +111,10 @@ private void assertSplitSignature(final List<SeekableByteChannel> channels)
* Concatenates the given channels.
*
* @param channels the channels to concatenate, note that the LAST CHANNEL of channels should be the LAST SEGMENT(.zip)
- * and theses channels should be added in correct order (e.g. .z01, .z02... .z99, .zip)
+ * and these channels should be added in correct order (e.g.: .z01, .z02... .z99, .zip)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if channels is null
- * @throws IOException if reading channels fails
+ * @throws IOException if reading channels fails
Review comment:
Ditto
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -75,4 +89,19 @@ public static String getBaseName(final String filename) {
return name.substring(0, extensionPosition);
}
+
+ public static String getBaseNameFrom(final Path path) {
Review comment:
As above plus needs a comment.
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir, "testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
final File fileToTest = getFile("COMPRESS-477/split_zip_created_by_zip/zip_to_compare_created_by_zip.zip");
final InputStream inputStream = Files.newInputStream(fileToTest.toPath());
final byte[] buffer = new byte[4096];
int readLen;
while ((readLen = inputStream.read(buffer)) > 0) {
- zipSplitOutputStream.write(buffer, 0, readLen);
+ ZipSplitOutputStream.write(buffer, 0, readLen);
Review comment:
-1 ditto
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -125,14 +128,14 @@ public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableB
*
* @param lastSegmentChannel channel of the last segment of split zip segments, its extension should be .zip
* @param channels the channels to concatenate except for the last segment,
- * note theses channels should be added in correct order (e.g. .z01, .z02... .z99)
+ * note these channels should be added in correct order (e.g.: .z01, .z02... .z99)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if lastSegmentChannel or channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableByteChannel lastSegmentChannel,
- final Iterable<SeekableByteChannel> channels) throws IOException {
+ final Iterable<SeekableByteChannel> channels) throws IOException {
Review comment:
Again, change messes up formatting.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775064100
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -365,8 +365,33 @@ public ZipArchiveOutputStream(final Path file, final OpenOption... options) thro
* @since 1.20
*/
public ZipArchiveOutputStream(final File file, final long zipSplitSize) throws IOException {
+ this(file.toPath(), zipSplitSize);
+ }
+
+ /**
+ * Creates a split ZIP Archive.
+ *
+ * <p>The files making up the archive will use Z01, Z02,
+ * ... extensions and the last part of it will be the given {@code
+ * file}.</p>
+ *
+ * <p>Even though the stream writes to a file this stream will
+ * behave as if no random access was possible. This means the
+ * sizes of stored entries need to be known before the actual
+ * entry data is written.</p>
+ *
+ * @param path the file that will become the last part of the split archive
+ * @param zipSplitSize maximum size of a single part of the split
+ * archive created by this stream. Must be between 64kB and about
+ * 4GB.
+ *
Review comment:
Tried my best to undo unrelated changes and keep new things compact and correct.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774550114
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -210,7 +222,21 @@ public ZipFile(final String name, final String encoding) throws IOException {
* @throws IOException if an error occurs while reading the file.
*/
public ZipFile(final File f, final String encoding) throws IOException {
- this(f, encoding, true);
+ this(f.toPath(), encoding, true);
+ }
+
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names and scanning for unicode extra fields.
+ *
+ * @param path path to the archive.
+ * @param encoding the encoding to use for file names, use null
+ * for the platform's default encoding
Review comment:
This is a copy paste from the existing javadocs of methods from within the class, if you want I can correct them all for white-spaces.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775064130
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Reverted.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774560315
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ParallelScatterZipCreator.java
##########
@@ -17,13 +17,11 @@
*/
package org.apache.commons.compress.archivers.zip;
-import org.apache.commons.compress.parallel.FileBasedScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.InputStreamSupplier;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStoreSupplier;
+import org.apache.commons.compress.parallel.*;
Review comment:
> We do not use wildcard imports. Please specify the packages you want to import.
Good catch, I think Checkstyle can catch that.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775080280
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
+ if (path == null) {
+ return null;
+ }
+
+ final String name = path.getFileName().toString();
+ final int extensionPosition = name.lastIndexOf('.');
+ if (extensionPosition < 0) {
Review comment:
Was going to comment on the public vs package/private here too, but I see @garydgregory already asked about it :point_up:
@PostelnicuGeorge I think we can change here to
```suggestion
if (extensionPosition <= 0 || extensionPosition == name.length() - 1) {
```
? This way we avoid calling `.substring` when the string is just a dot, or ends with a dot.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774536136
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,19 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ public static String getExtensionFrom(final Path path) {
Review comment:
My intention with these 2 methods getExtensionFrom and getBaseNameFrom is to eventually replace getExtension and getBaseName, given the transition to Paths, furthermore they can't be private/package-private due to their usage in tests and in other packages.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774599878
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -75,4 +89,19 @@ public static String getBaseName(final String filename) {
return name.substring(0, extensionPosition);
}
+
+ public static String getBaseNameFrom(final Path path) {
Review comment:
Added javadocs
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775047837
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Please keep the PR focused on one thing, this is a behavior change unrelated to NIO. May you revert this line please?
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Please keep the PR focused on one thing, this is a behavior change unrelated to NIO. May you revert this line, please?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775080280
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
+ if (path == null) {
+ return null;
+ }
+
+ final String name = path.getFileName().toString();
+ final int extensionPosition = name.lastIndexOf('.');
+ if (extensionPosition < 0) {
Review comment:
Was going to comment on the public vs package/private here too, but I see @garydgregory already asked about it :point_up:
@PostelnicuGeorge I think we can change here to
```suggestion
if (extensionPosition <= 0 || extensionPosition == name.length() - 1) {
```
? This way we avoid calling `.substring` when the strings ends with a dot.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - Migrate zip package to use NIO
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775251767
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -70,8 +80,8 @@ public void close() throws IOException {
try {
closeForWriting();
} finally {
- if (target.exists() && !target.delete()) {
- target.deleteOnExit();
+ if (Files.exists(target)) {
+ Files.deleteIfExists(target);
Review comment:
Hello @PostelnicuGeorge
Why are you checking if the file exists twice? Isn't this the whole point of using `Files.deleteIfExists()`? :wink:
Also, the behavior is now different: The old behavior guaranteed file deletion, this one does not. Previously, if the file was not deleted, then it was scheduled for deletion on VM exit.
The same is usually achieved by using `StandardOpenOption.DELETE_ON_CLOSE` on open but in this case, it won't work because our use case is... complicated. This might be worth addressing in a follow-up PR though.
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -75,4 +99,29 @@ public static String getBaseName(final String filename) {
return name.substring(0, extensionPosition);
}
+
+ /**
+ * Returns the basename (i.e. the part up to and not including the
+ * last ".") of the last path segment of a filename.
+ * <p>Will return the file name itself if it doesn't contain any
+ * dots. All leading directories of the {@code filename} parameter
+ * are skipped.</p>
+ * @return the basename of filename
+ * @param path the path of the file to obtain the basename of.
+ * @since 1.22
+ */
+ public static String getBaseNameFrom(final Path path) {
Review comment:
This is a new method but it does NOT have a unit test like the existing String version of the method does in `FileNameUtilsTest`.
You can just shadow the existing `getBaseName` method name here, no need to invent a new naming style. You can just typecast any existing null invocations to the old API argument.
Ouch, this class now has too much code duplication. The same pattern is present four times.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -155,30 +158,40 @@ public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableB
* the beginning of a split archive
*/
public static SeekableByteChannel buildFromLastSplitSegment(final File lastSegmentFile) throws IOException {
- final String extension = FileNameUtils.getExtension(lastSegmentFile.getCanonicalPath());
+ return buildFromLastSplitSegment(lastSegmentFile.toPath());
Review comment:
Hm, I wonder if there are any unintended side effects of switching from `getCanonicalPath()` to `toPath()` aside from the type change.
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
Review comment:
This is a new method but it does NOT have a unit test like the existing String version of the method does in `FileNameUtilsTest`.
You can just shadow the existing `getExtension` method name here, no need to invent a new naming style. You can just typecast any existing null invocations to the old API argument.
Hm... this class now has too much code duplication. The same pattern is present four times.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -193,9 +206,29 @@ public static SeekableByteChannel buildFromLastSplitSegment(final File lastSegme
* the beginning of a split archive
*/
public static SeekableByteChannel forFiles(final File... files) throws IOException {
- final List<SeekableByteChannel> channels = new ArrayList<>();
+ final List<Path> paths = new ArrayList<>();
for (final File f : Objects.requireNonNull(files, "files must not be null")) {
- channels.add(Files.newByteChannel(f.toPath(), StandardOpenOption.READ));
+ paths.add(f.toPath());
+ }
+
+ return forPaths(paths.toArray(new Path[0]));
Review comment:
Extract a constant here.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -218,21 +251,45 @@ public static SeekableByteChannel forFiles(final File lastSegmentFile, final Ite
Objects.requireNonNull(files, "files");
Objects.requireNonNull(lastSegmentFile, "lastSegmentFile");
- final List<File> filesList = new ArrayList<>();
+ final List<Path> filesList = new ArrayList<>();
for (final File f : files) {
+ filesList.add(f.toPath());
+ }
+
+ return forPaths(lastSegmentFile.toPath(), filesList);
+ }
+
+ /**
+ * Concatenates the given file paths.
+ * @param lastSegmentPath the last segment path of split zip segments, its extension must be .zip
+ * @param paths the file paths to concatenate except for the last segment,
+ * note these files should be added in correct order (e.g.: .z01, .z02... .z99)
+ * @return SeekableByteChannel that concatenates all provided files
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
+ * @throws NullPointerException if files or lastSegmentPath is null
+ * @since 1.22
+ */
+ public static SeekableByteChannel forPaths(final Path lastSegmentPath, final Iterable<Path> paths) throws IOException {
+ Objects.requireNonNull(paths, "paths");
+ Objects.requireNonNull(lastSegmentPath, "lastSegmentPath");
+
+ final List<Path> filesList = new ArrayList<>();
+ for (final Path f : paths) {
filesList.add(f);
}
- filesList.add(lastSegmentFile);
+ filesList.add(lastSegmentPath);
- return forFiles(filesList.toArray(new File[0]));
+ return forPaths(filesList.toArray(new Path[0]));
Review comment:
Extract a constant here.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -149,12 +161,9 @@ private void finish() throws IOException {
throw new IOException("This archive has already been finished");
}
- final String zipFileBaseName = FileNameUtils.getBaseName(zipFile.getName());
- final File lastZipSplitSegmentFile = new File(zipFile.getParentFile(), zipFileBaseName + ".zip");
+ final String zipFileBaseName = FileNameUtils.getBaseName(zipFile.getFileName().toString());
Review comment:
This PR just created a Path version of this API, so reuse it instead of converting.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775080280
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
+ if (path == null) {
+ return null;
+ }
+
+ final String name = path.getFileName().toString();
+ final int extensionPosition = name.lastIndexOf('.');
+ if (extensionPosition < 0) {
Review comment:
Was going to comment on the public vs package/private here too, but I see @garydgregory already asked about it :point_up:
@PostelnicuGeorge I think we can change here to
```suggestion
if (extensionPosition < 0 || extensionPosition == name.length() - 1) {
```
? This way we avoid calling `.substring` when the strings ends with a dot.
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -155,30 +158,40 @@ public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableB
* the beginning of a split archive
*/
public static SeekableByteChannel buildFromLastSplitSegment(final File lastSegmentFile) throws IOException {
- final String extension = FileNameUtils.getExtension(lastSegmentFile.getCanonicalPath());
+ return buildFromLastSplitSegment(lastSegmentFile.toPath());
+ }
+
+ /**
+ * Concatenates zip split files from the last segment(the extension SHOULD be .zip)
Review comment:
Missing space between segment and parenthesis, and would SHOULD be replaced by MUST? It repeats in other javadocs further down in this file :point_down:
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775070689
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -23,21 +23,26 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
+import java.nio.file.Path;
/**
- * ScatterGatherBackingStore that is backed by a file.
+ * ScatterGatherBackingStore that is backed by a file or path.
*
- * @since 1.10
+ * @since 1.22
*/
public class FileBasedScatterGatherBackingStore implements ScatterGatherBackingStore {
- private final File target;
+ private final Path target;
private final OutputStream os;
private boolean closed;
public FileBasedScatterGatherBackingStore(final File target) throws FileNotFoundException {
+ this(target.toPath());
+ }
+
+ public FileBasedScatterGatherBackingStore(final Path target) throws FileNotFoundException {
Review comment:
Reverted change to class level javadoc, added a new one at constructor level with since tag.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory closed pull request #236: COMPRESS-602 - Migrate zip package to use NIO
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #236:
URL: https://github.com/apache/commons-compress/pull/236
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#issuecomment-1000890010
Hi @PostelnicuGeorge
Thank you for your updates.
When I look at the PR as a [diff](https://patch-diff.githubusercontent.com/raw/apache/commons-compress/pull/236.diff) I still see missing Javadoc since tags.
Let's keep this PR focused on just NIO without other changes.
Please see my other comments.
TY!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#issuecomment-1000912207
Hi @garydgregory ,
Tried my best to revert all changes that are unrelated to NIO, keep the javadocs compact and correct, code is updated with latest master as well.
Let me know if I missed other things.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774539120
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -57,19 +58,31 @@
* @param zipFile the zip file to write to
* @param splitSize the split size
*/
- public ZipSplitOutputStream(final File zipFile, final long splitSize) throws IllegalArgumentException, IOException {
+ public ZipSplitOutputStream(final Path zipFile, final long splitSize) throws IllegalArgumentException, IOException {
if (splitSize < ZIP_SEGMENT_MIN_SIZE || splitSize > ZIP_SEGMENT_MAX_SIZE) {
throw new IllegalArgumentException("zip split segment size should between 64K and 4,294,967,295");
}
this.zipFile = zipFile;
this.splitSize = splitSize;
- this.outputStream = Files.newOutputStream(zipFile.toPath());
+ this.outputStream = Files.newOutputStream(zipFile);
// write the zip split signature 0x08074B50 to the zip file
writeZipSplitSignature();
}
+ /**
+ * Create a split zip. If the zip file is smaller than the split size,
+ * then there will only be one split zip, and its suffix is .zip,
+ * otherwise the split segments should be like .z01, .z02, ... .z(N-1), .zip
+ *
+ * @param zipFile the zip file to write to
+ * @param splitSize the split size
+ */
Review comment:
@garydgregory I don't understand your comment, can you elaborate cause you mentioned this kind of change multiple times and I don't want to create more problems.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#issuecomment-1000311609
Added since tags to all the new public methods I have added, hopefully I haven't missed something, I have also removed all .* star imports and removed all the white-spaces added by my formatting.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775068262
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -23,21 +23,26 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
+import java.nio.file.Path;
/**
- * ScatterGatherBackingStore that is backed by a file.
+ * ScatterGatherBackingStore that is backed by a file or path.
*
- * @since 1.10
+ * @since 1.22
*/
public class FileBasedScatterGatherBackingStore implements ScatterGatherBackingStore {
- private final File target;
+ private final Path target;
private final OutputStream os;
private boolean closed;
public FileBasedScatterGatherBackingStore(final File target) throws FileNotFoundException {
+ this(target.toPath());
+ }
+
+ public FileBasedScatterGatherBackingStore(final Path target) throws FileNotFoundException {
Review comment:
I changed in place the javadoc at class level, do you want me to put it at the constructor level?
```java
/**
* ScatterGatherBackingStore that is backed by a file or path.
*
* @since 1.22
*/
```
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775067730
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -23,21 +23,26 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
+import java.nio.file.Path;
/**
- * ScatterGatherBackingStore that is backed by a file.
+ * ScatterGatherBackingStore that is backed by a file or path.
*
- * @since 1.10
+ * @since 1.22
*/
public class FileBasedScatterGatherBackingStore implements ScatterGatherBackingStore {
- private final File target;
+ private final Path target;
private final OutputStream os;
private boolean closed;
public FileBasedScatterGatherBackingStore(final File target) throws FileNotFoundException {
+ this(target.toPath());
+ }
+
+ public FileBasedScatterGatherBackingStore(final Path target) throws FileNotFoundException {
Review comment:
This new element needs a Javadoc comment and since tag.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775047520
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -365,8 +365,33 @@ public ZipArchiveOutputStream(final Path file, final OpenOption... options) thro
* @since 1.20
*/
public ZipArchiveOutputStream(final File file, final long zipSplitSize) throws IOException {
+ this(file.toPath(), zipSplitSize);
+ }
+
+ /**
+ * Creates a split ZIP Archive.
+ *
+ * <p>The files making up the archive will use Z01, Z02,
+ * ... extensions and the last part of it will be the given {@code
+ * file}.</p>
+ *
+ * <p>Even though the stream writes to a file this stream will
+ * behave as if no random access was possible. This means the
+ * sizes of stored entries need to be known before the actual
+ * entry data is written.</p>
+ *
+ * @param path the file that will become the last part of the split archive
+ * @param zipSplitSize maximum size of a single part of the split
+ * archive created by this stream. Must be between 64kB and about
+ * 4GB.
+ *
Review comment:
I prefer a focused PR, new lines should be "correct". If you want to update unrelated lines, feel free to do so in a _different_ PR, please. This makes reviewing simpler and the PR itself only about one item.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775142589
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
+ if (path == null) {
+ return null;
+ }
+
+ final String name = path.getFileName().toString();
+ final int extensionPosition = name.lastIndexOf('.');
+ if (extensionPosition < 0) {
Review comment:
The package private can't be because of the usages and due to the fact that I would like that these 2 methods will replace the original ones in the tests and in the main code usages. Also I didn't try to change the functionality that was there before only to make it work for nio, that's why I didn't attempt any enhancements anywhere only equivalent methods from io to nio.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775142475
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -155,30 +158,40 @@ public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableB
* the beginning of a split archive
*/
public static SeekableByteChannel buildFromLastSplitSegment(final File lastSegmentFile) throws IOException {
- final String extension = FileNameUtils.getExtension(lastSegmentFile.getCanonicalPath());
+ return buildFromLastSplitSegment(lastSegmentFile.toPath());
+ }
+
+ /**
+ * Concatenates zip split files from the last segment(the extension SHOULD be .zip)
Review comment:
hey, I changed in the places were I changed, but won't change in the places that were before, like Gary said to keep modifications only on the scope of the task.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774550296
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java
##########
@@ -365,8 +365,33 @@ public ZipArchiveOutputStream(final Path file, final OpenOption... options) thro
* @since 1.20
*/
public ZipArchiveOutputStream(final File file, final long zipSplitSize) throws IOException {
+ this(file.toPath(), zipSplitSize);
+ }
+
+ /**
+ * Creates a split ZIP Archive.
+ *
+ * <p>The files making up the archive will use Z01, Z02,
+ * ... extensions and the last part of it will be the given {@code
+ * file}.</p>
+ *
+ * <p>Even though the stream writes to a file this stream will
+ * behave as if no random access was possible. This means the
+ * sizes of stored entries need to be known before the actual
+ * entry data is written.</p>
+ *
+ * @param path the file that will become the last part of the split archive
+ * @param zipSplitSize maximum size of a single part of the split
+ * archive created by this stream. Must be between 64kB and about
+ * 4GB.
+ *
Review comment:
This is a copy paste from the existing javadocs of methods from within the class, if you want I can correct them all for white-spaces.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - Migrate zip package to use NIO
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775398732
##########
File path: src/main/java/org/apache/commons/compress/parallel/FileBasedScatterGatherBackingStore.java
##########
@@ -70,8 +80,8 @@ public void close() throws IOException {
try {
closeForWriting();
} finally {
- if (target.exists() && !target.delete()) {
- target.deleteOnExit();
+ if (Files.exists(target)) {
+ Files.deleteIfExists(target);
Review comment:
Hey Gary you're right, I've read about StandardOpenOption.DELETE_ON_CLOSE and it only works in the same try-with-resources, the way we're passing streams around doesn't so I think we need to think about a general solution or change the way we're using different things.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774556478
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -125,14 +128,14 @@ public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableB
*
* @param lastSegmentChannel channel of the last segment of split zip segments, its extension should be .zip
* @param channels the channels to concatenate except for the last segment,
- * note theses channels should be added in correct order (e.g. .z01, .z02... .z99)
+ * note these channels should be added in correct order (e.g.: .z01, .z02... .z99)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if lastSegmentChannel or channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public static SeekableByteChannel forOrderedSeekableByteChannels(final SeekableByteChannel lastSegmentChannel,
- final Iterable<SeekableByteChannel> channels) throws IOException {
+ final Iterable<SeekableByteChannel> channels) throws IOException {
Review comment:
My mistake, reverted!
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -108,10 +111,10 @@ private void assertSplitSignature(final List<SeekableByteChannel> channels)
* Concatenates the given channels.
*
* @param channels the channels to concatenate, note that the LAST CHANNEL of channels should be the LAST SEGMENT(.zip)
- * and theses channels should be added in correct order (e.g. .z01, .z02... .z99, .zip)
+ * and these channels should be added in correct order (e.g.: .z01, .z02... .z99, .zip)
* @return SeekableByteChannel that concatenates all provided channels
* @throws NullPointerException if channels is null
- * @throws IOException if reading channels fails
+ * @throws IOException if reading channels fails
Review comment:
My mistake, reverted!
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
*/
public ZipSplitReadOnlySeekableByteChannel(final List<SeekableByteChannel> channels)
- throws IOException {
+ throws IOException {
Review comment:
My mistake, reverted!
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -58,11 +61,11 @@
*
* @param channels the channels to concatenate
* @throws NullPointerException if channels is null
- * @throws IOException if the first channel doesn't seem to hold
- * the beginning of a split archive
+ * @throws IOException if the first channel doesn't seem to hold
+ * the beginning of a split archive
Review comment:
My mistake, reverted!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774557487
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -57,19 +58,31 @@
* @param zipFile the zip file to write to
* @param splitSize the split size
*/
- public ZipSplitOutputStream(final File zipFile, final long splitSize) throws IllegalArgumentException, IOException {
+ public ZipSplitOutputStream(final Path zipFile, final long splitSize) throws IllegalArgumentException, IOException {
if (splitSize < ZIP_SEGMENT_MIN_SIZE || splitSize > ZIP_SEGMENT_MAX_SIZE) {
throw new IllegalArgumentException("zip split segment size should between 64K and 4,294,967,295");
}
this.zipFile = zipFile;
this.splitSize = splitSize;
- this.outputStream = Files.newOutputStream(zipFile.toPath());
+ this.outputStream = Files.newOutputStream(zipFile);
// write the zip split signature 0x08074B50 to the zip file
writeZipSplitSignature();
}
+ /**
+ * Create a split zip. If the zip file is smaller than the split size,
+ * then there will only be one split zip, and its suffix is .zip,
+ * otherwise the split segments should be like .z01, .z02, ... .z(N-1), .zip
+ *
+ * @param zipFile the zip file to write to
+ * @param splitSize the split size
+ */
Review comment:
Hi @PostelnicuGeorge
@garydgregory is talking about the `@since` tag. You can also find this in comments of other methods.
1.21 is already released so you should add a `@since 1.22` here.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774558203
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java
##########
@@ -57,19 +58,31 @@
* @param zipFile the zip file to write to
* @param splitSize the split size
*/
- public ZipSplitOutputStream(final File zipFile, final long splitSize) throws IllegalArgumentException, IOException {
+ public ZipSplitOutputStream(final Path zipFile, final long splitSize) throws IllegalArgumentException, IOException {
if (splitSize < ZIP_SEGMENT_MIN_SIZE || splitSize > ZIP_SEGMENT_MAX_SIZE) {
throw new IllegalArgumentException("zip split segment size should between 64K and 4,294,967,295");
}
this.zipFile = zipFile;
this.splitSize = splitSize;
- this.outputStream = Files.newOutputStream(zipFile.toPath());
+ this.outputStream = Files.newOutputStream(zipFile);
// write the zip split signature 0x08074B50 to the zip file
writeZipSplitSignature();
}
+ /**
+ * Create a split zip. If the zip file is smaller than the split size,
+ * then there will only be one split zip, and its suffix is .zip,
+ * otherwise the split segments should be like .z01, .z02, ... .z(N-1), .zip
+ *
+ * @param zipFile the zip file to write to
+ * @param splitSize the split size
+ */
Review comment:
Thank you so much, on it!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - Migrate zip package to use NIO
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r775399688
##########
File path: src/main/java/org/apache/commons/compress/utils/FileNameUtils.java
##########
@@ -50,6 +51,29 @@ public static String getExtension(final String filename) {
return name.substring(extensionPosition + 1);
}
+ /**
+ * Returns the extension (i.e. the part after the last ".") of a file.
+ * <p>Will return an empty string if the file name doesn't contain
+ * any dots. Only the last segment of a the file name is consulted
+ * - i.e. all leading directories of the {@code filename}
+ * parameter are skipped.</p>
+ * @return the extension of filename
+ * @param path the path of the file to obtain the extension of.
+ * @since 1.22
+ */
+ public static String getExtensionFrom(final Path path) {
Review comment:
I thought of them as utility methods and it didn't even occur to me to unit test them. Will do so in the future!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774540548
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ScatterZipOutputStreamTest.java
##########
@@ -45,7 +46,8 @@ public void cleanup() {
@Test
public void putArchiveEntry() throws Exception {
scatterFile = File.createTempFile("scattertest", ".notzip");
- final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile);
+ final ScatterZipOutputStream scatterZipOutputStream = ScatterZipOutputStream.fileBased(scatterFile,
+ Deflater.BEST_COMPRESSION);
final byte[] B_PAYLOAD = "RBBBBBBS".getBytes();
Review comment:
Wanted to increase the coverage and test this branch as well.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774575510
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ParallelScatterZipCreator.java
##########
@@ -17,13 +17,11 @@
*/
package org.apache.commons.compress.archivers.zip;
-import org.apache.commons.compress.parallel.FileBasedScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.InputStreamSupplier;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStore;
-import org.apache.commons.compress.parallel.ScatterGatherBackingStoreSupplier;
+import org.apache.commons.compress.parallel.*;
Review comment:
I have undone all .* imports in the whole zip package
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774549770
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
##########
@@ -262,6 +305,39 @@ public ZipFile(final File f, final String encoding, final boolean useUnicodeExtr
f.getAbsolutePath(), encoding, useUnicodeExtraFields, true, ignoreLocalFileHeader);
}
+ /**
+ * Opens the given path for reading, assuming the specified
+ * encoding for file names.
+ *
+ *
Review comment:
This is a copy paste from the existing javadocs of methods from within the class, if you want I can correct them all for white-spaces.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774538442
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir, "testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
Review comment:
My mistake, don't know how this happened or why I modified this test :/
##########
File path: src/test/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStreamTest.java
##########
@@ -69,19 +69,19 @@ public void splitZipBeginsWithZipSplitSignature() throws IOException {
public void testCreateSplittedFiles() throws IOException {
final File testOutputFile = new File(dir, "testCreateSplittedFiles.zip");
final int splitSize = 100 * 1024; /* 100KB */
- final ZipSplitOutputStream zipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
+ final ZipSplitOutputStream ZipSplitOutputStream = new ZipSplitOutputStream(testOutputFile, splitSize);
final File fileToTest = getFile("COMPRESS-477/split_zip_created_by_zip/zip_to_compare_created_by_zip.zip");
final InputStream inputStream = Files.newInputStream(fileToTest.toPath());
final byte[] buffer = new byte[4096];
int readLen;
while ((readLen = inputStream.read(buffer)) > 0) {
- zipSplitOutputStream.write(buffer, 0, readLen);
+ ZipSplitOutputStream.write(buffer, 0, readLen);
Review comment:
My mistake, don't know how this happened or why I modified this test :/
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PostelnicuGeorge commented on a change in pull request #236: COMPRESS-602 - migrate zip package to nio path
Posted by GitBox <gi...@apache.org>.
PostelnicuGeorge commented on a change in pull request #236:
URL: https://github.com/apache/commons-compress/pull/236#discussion_r774556741
##########
File path: src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitReadOnlySeekableByteChannel.java
##########
@@ -47,7 +50,7 @@
public class ZipSplitReadOnlySeekableByteChannel extends MultiReadOnlySeekableByteChannel {
private static final int ZIP_SPLIT_SIGNATURE_LENGTH = 4;
private final ByteBuffer zipSplitSignatureByteBuffer =
- ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
+ ByteBuffer.allocate(ZIP_SPLIT_SIGNATURE_LENGTH);
Review comment:
My mistake, reverted!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org