[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r321902124
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CheckedBiFunction.java
##
@@ -0,0 +1,29 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.util;
+
+import java.io.IOException;
+
+/**
+ * Defines a functional interface having two inputs which throws IOException.
+ */
+@FunctionalInterface
+public interface CheckedBiFunction
{
Review comment:
yeah, they should. I'm doing in new code that I know isn't going to be
backportable into jdk7, not now we have to worry about jdk11
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, options) is called. URL: https://github.com/apache/hadoop/pull/1388#discussion_r320405606 ## File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestChecksumFs.java ## @@ -0,0 +1,130 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs; + +import java.io.IOException; +import java.util.EnumSet; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.local.LocalFs; +import org.apache.hadoop.fs.permission.FsPermission; +import static org.apache.hadoop.fs.CreateFlag.*; +import org.apache.hadoop.test.GenericTestUtils; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.*; Review comment: order should be ``` java all-not org.apache, non-static org.apache. all static imports ``` and in alphabetical order in blocks. It's not enforced the way scalacheck does for spark, but we try to have a rule, even if gets neglected. That static import block is critical. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320403035
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFs.java
##
@@ -457,17 +458,35 @@ public boolean setReplication(Path src, short
replication)
@Override
public void renameInternal(Path src, Path dst)
throws IOException, UnresolvedLinkException {
+renameInternal(src, dst, (s, d) -> getMyFs().rename(s, d));
+ }
+
+ @Override
+ public void renameInternal(Path src, Path dst, boolean overwrite)
+ throws AccessControlException, FileAlreadyExistsException,
+ FileNotFoundException, ParentNotDirectoryException,
+ UnresolvedLinkException, IOException {
+Options.Rename renameOpt = Options.Rename.NONE;
+if (overwrite) {
+ renameOpt = Options.Rename.OVERWRITE;
+}
+final Options.Rename opt = renameOpt;
+renameInternal(src, dst, (s, d) -> getMyFs().rename(s, d, opt));
Review comment:
Looking at this, its the more elegant functional API. Which is nice for
hadoop 3+. But I fear its probably going to lose all that elegance on branch-2
(assuming you do want a backport). If you do, then simply copying the existing
renameInternal to one with a new signature is going to be the simplest
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320399574
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestChecksumFs.java
##
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs;
+
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.local.LocalFs;
+import org.apache.hadoop.fs.permission.FsPermission;
+import static org.apache.hadoop.fs.CreateFlag.*;
+import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+/**
+ * This class tests the functionality of ChecksumFs.
+ */
+public class TestChecksumFs {
+ private Configuration conf;
+ private Path testRootDirPath;
+ private FileContext fc;
+
+ @Before
+ public void setUp() throws Exception {
+conf = getTestConfiguration();
+fc = FileContext.getFileContext(conf);
+testRootDirPath = new Path(GenericTestUtils.getRandomizedTestDir()
+.getAbsolutePath());
+mkdirs(testRootDirPath);
+ }
+
+ public void tearDown() throws Exception {
+fc.delete(testRootDirPath, true);
+ }
+
+ @Test
+ public void testRenameFileToFile() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDst");
+verifyRename(srcPath, dstPath, false);
+ }
+
+ @Test
+ public void testRenameFileToFileWithOverwrite() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDst");
+verifyRename(srcPath, dstPath, true);
+ }
+
+ @Test
+ public void testRenameFileIntoDirFile() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDir/testRenameDst");
+mkdirs(dstPath);
+verifyRename(srcPath, dstPath, false);
+ }
+
+ @Test
+ public void testRenameFileIntoDirFileWithOverwrite() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDir/testRenameDst");
+mkdirs(dstPath);
+verifyRename(srcPath, dstPath, true);
+ }
+
+ private void verifyRename(Path srcPath, Path dstPath,
+ boolean overwrite) throws Exception {
+AbstractFileSystem fs = fc.getDefaultFileSystem();
+assertTrue(fs instanceof LocalFs);
+ChecksumFs checksumFs = (ChecksumFs) fs;
+
+fs.delete(srcPath, true);
+fs.delete(dstPath, true);
+
+Options.Rename renameOpt = Options.Rename.NONE;
+if (overwrite) {
+ renameOpt = Options.Rename.OVERWRITE;
+ createTestFile(checksumFs, dstPath, 2);
+}
+
+// ensure file + checksum are moved
+createTestFile(checksumFs, srcPath, 1);
+assertTrue(fc.util().exists(checksumFs.getChecksumFile(srcPath)));
Review comment:
can the assert add text as to what is failing?
FWIW, we are moving to AssertJ for 3.2+ suites, but it would make back
porting near-impossible. Just stick to junit asserts, but consider "what
information would I want in a jenkins run to debug a failure?"
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320397813
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestChecksumFs.java
##
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs;
+
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.local.LocalFs;
+import org.apache.hadoop.fs.permission.FsPermission;
+import static org.apache.hadoop.fs.CreateFlag.*;
+import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+/**
+ * This class tests the functionality of ChecksumFs.
+ */
+public class TestChecksumFs {
+ private Configuration conf;
+ private Path testRootDirPath;
+ private FileContext fc;
+
+ @Before
+ public void setUp() throws Exception {
+conf = getTestConfiguration();
+fc = FileContext.getFileContext(conf);
+testRootDirPath = new Path(GenericTestUtils.getRandomizedTestDir()
+.getAbsolutePath());
+mkdirs(testRootDirPath);
+ }
+
+ public void tearDown() throws Exception {
+fc.delete(testRootDirPath, true);
+ }
+
+ @Test
+ public void testRenameFileToFile() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDst");
+verifyRename(srcPath, dstPath, false);
+ }
+
+ @Test
+ public void testRenameFileToFileWithOverwrite() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDst");
+verifyRename(srcPath, dstPath, true);
+ }
+
+ @Test
+ public void testRenameFileIntoDirFile() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDir/testRenameDst");
+mkdirs(dstPath);
+verifyRename(srcPath, dstPath, false);
+ }
+
+ @Test
+ public void testRenameFileIntoDirFileWithOverwrite() throws Exception {
+Path srcPath = new Path(testRootDirPath, "testRenameSrc");
+Path dstPath = new Path(testRootDirPath, "testRenameDir/testRenameDst");
+mkdirs(dstPath);
+verifyRename(srcPath, dstPath, true);
+ }
+
+ private void verifyRename(Path srcPath, Path dstPath,
+ boolean overwrite) throws Exception {
+AbstractFileSystem fs = fc.getDefaultFileSystem();
+assertTrue(fs instanceof LocalFs);
Review comment:
just cast it; you'll get a better stack trace that way
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320397235
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestChecksumFs.java
##
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs;
+
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.local.LocalFs;
+import org.apache.hadoop.fs.permission.FsPermission;
+import static org.apache.hadoop.fs.CreateFlag.*;
+import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+/**
+ * This class tests the functionality of ChecksumFs.
+ */
+public class TestChecksumFs {
+ private Configuration conf;
+ private Path testRootDirPath;
+ private FileContext fc;
+
+ @Before
+ public void setUp() throws Exception {
+conf = getTestConfiguration();
+fc = FileContext.getFileContext(conf);
+testRootDirPath = new Path(GenericTestUtils.getRandomizedTestDir()
+.getAbsolutePath());
+mkdirs(testRootDirPath);
+ }
+
+ public void tearDown() throws Exception {
Review comment:
@After; must handle case where fc == null
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320396970
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestChecksumFs.java
##
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs;
+
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.local.LocalFs;
+import org.apache.hadoop.fs.permission.FsPermission;
+import static org.apache.hadoop.fs.CreateFlag.*;
+import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+/**
+ * This class tests the functionality of ChecksumFs.
+ */
+public class TestChecksumFs {
Review comment:
If you don;t extend org.apache.hadoop.test.HadoopTestBase, look at that
class and know that a timeout rule is not optional.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1388: HADOOP-16255. Add ChecksumFs.rename(path, path, boolean) to rename crc file as well when FileContext.rename(path, path, opt
steveloughran commented on a change in pull request #1388: HADOOP-16255. Add
ChecksumFs.rename(path, path, boolean) to rename crc file as well when
FileContext.rename(path, path, options) is called.
URL: https://github.com/apache/hadoop/pull/1388#discussion_r320396059
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CheckedBiFunction.java
##
@@ -0,0 +1,29 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.util;
+
+import java.io.IOException;
+
+/**
+ * Defines a functional interface having two inputs which throws IOException.
+ */
+@FunctionalInterface
+public interface CheckedBiFunction
{
Review comment:
Cute! I never knew you could do that with templates and exceptions!
1. can you put into org.apache.hadoop.fs.impl where the other
internal-for-fs-only lambda stuff is going.
1. be advised that for backports to branch 2 we will have to make things
compile on Java 8. Mostly this is just using the IDE to convert things to
callables. Doesn't mean they shouldn't be used, only that once you get
sufficiently advanced things become unbackportable. This patch looks fine
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
