[GitHub] [hadoop] steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r395740845
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java
##
@@ -76,12 +79,8 @@ public void testAbfsStreamOps() throws Exception {
assertReadWriteOps("read", 1, statistics.getReadOps());
} finally {
- if (inForOneOperation != null) {
-inForOneOperation.close();
- }
- if (outForOneOperation != null) {
-outForOneOperation.close();
- }
+ IOUtils.cleanupWithLogger(null, inForOneOperation,
Review comment:
you are going to need to create a logger in this test case and pass it down
i'm afraid
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r394273761
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemOauth.java
##
@@ -143,7 +143,7 @@ public void testBlobDataReader() throws Exception {
// TEST WRITE FILE
try {
- abfsStore.openFileForWrite(EXISTED_FILE_PATH, true);
+ abfsStore.openFileForWrite(EXISTED_FILE_PATH, fs.getFsStatistics(),
true);
Review comment:
add a .close() at the end so if something went wrong and the file was
opened, we close the stream
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r394273198
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java
##
@@ -0,0 +1,147 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+
+/**
+ * Test Abfs Stream.
+ */
+
+public class ITestAbfsStreamStatistics extends AbstractAbfsIntegrationTest {
+ public ITestAbfsStreamStatistics() throws Exception {
+ }
+
+ /***
+ * Testing {@code incrementReadOps()} in class {@code AbfsInputStream} and
+ * {@code incrementWriteOps()} in class {@code AbfsOutputStream}.
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testAbfsStreamOps() throws Exception {
+describe("Test to see correct population of read and write operations in "
++ "Abfs");
+
+final AzureBlobFileSystem fs = getFileSystem();
+Path smallOperationsFile = new Path("testOneReadWriteOps");
+Path largeOperationsFile = new Path("testLargeReadWriteOps");
+FileSystem.Statistics statistics = fs.getFsStatistics();
+String testReadWriteOps = "test this";
+statistics.reset();
+
+//Test for zero write operation
+assertReadWriteOps("write", 0, statistics.getWriteOps());
+
+//Test for zero read operation
+assertReadWriteOps("read", 0, statistics.getReadOps());
+
+FSDataOutputStream outForOneOperation = null;
+FSDataInputStream inForOneOperation = null;
+try {
+ outForOneOperation = fs.create(smallOperationsFile);
+ statistics.reset();
+ outForOneOperation.write(testReadWriteOps.getBytes());
+
+ //Test for a single write operation
+ assertReadWriteOps("write", 1, statistics.getWriteOps());
+
+ inForOneOperation = fs.open(smallOperationsFile);
+ inForOneOperation.read(testReadWriteOps.getBytes(), 0,
+ testReadWriteOps.getBytes().length);
+
+ //Test for a single read operation
+ assertReadWriteOps("read", 1, statistics.getReadOps());
+
+} finally {
+ if (inForOneOperation != null) {
+inForOneOperation.close();
+ }
+ if (outForOneOperation != null) {
+outForOneOperation.close();
+ }
+}
+
+//Validating if content is being written in the smallOperationsFile
+Assert.assertTrue("Mismatch in content validation",
+validateContent(fs, smallOperationsFile,
+testReadWriteOps.getBytes()));
+
+FSDataOutputStream outForLargeOperations = null;
+FSDataInputStream inForLargeOperations = null;
+StringBuilder largeOperationsValidationString = new StringBuilder();
+try {
+ outForLargeOperations = fs.create(largeOperationsFile);
+ statistics.reset();
+ int largeValue = 100;
+ for (int i = 0; i < largeValue; i++) {
+outForLargeOperations.write(testReadWriteOps.getBytes());
+
+//Creating the String for content Validation
+largeOperationsValidationString.append(testReadWriteOps);
+ }
+
+ //Test for 100 write operations
+ assertReadWriteOps("write", largeValue, statistics.getWriteOps());
+
+ inForLargeOperations = fs.open(largeOperationsFile);
+ for (int i = 0; i < largeValue; i++)
+inForLargeOperations
+.read(testReadWriteOps.getBytes(), 0,
+testReadWriteOps.getBytes().length);
+
+ //Test for 100 read operations
+ assertReadWriteOps("read", largeValue, statistics.getReadOps());
+
+} finally {
+ if (inForLargeOperations != null) {
Review comment:
```
IOUtils.cleanupWithLogger(LOG, inForLargeOperations, outForLargeOperations)
```
that does close on all non-null arguments, catches failures so they don't
get in the way of whatever was thrown earlier. We use this throughout the
hadoop codebase to clean up robustly, so get used to it
[GitHub] [hadoop] steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r394271627
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java
##
@@ -67,4 +77,46 @@ public void nameThread() {
protected int getTestTimeoutMillis() {
return TEST_TIMEOUT;
}
+
+ /**
+ * Describe a test in the logs.
+ *
+ * @param text text to print
+ * @param args arguments to format in the printing
+ */
+ protected void describe(String text, Object... args) {
+LOG.info("\n\n{}: {}\n",
+methodName.getMethodName(),
+String.format(text, args));
+ }
+
+ /**
+ * Validate Contents written on a file in Abfs.
+ *
+ * @param fsAzureBlobFileSystem
+ * @param path Path of the file
+ * @param originalByteArray original byte array
+ * @return
+ * @throws IOException
+ */
+ protected boolean validateContent(AzureBlobFileSystem fs, Path path,
+ byte[] originalByteArray)
+ throws IOException {
+FSDataInputStream in = fs.open(path);
Review comment:
needs to be closed. you can use `try (FSDataInputStream ...) { }` to manage
this automatically
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS URL: https://github.com/apache/hadoop/pull/1881#discussion_r392964448 ## File path: hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java ## @@ -0,0 +1,111 @@ +/** + * 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.azurebfs; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.FileSystem; Review comment: minor: nit: ordering 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392958582
##
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java
##
@@ -181,6 +185,14 @@ public synchronized void write(final byte[] data, final
int off, final int lengt
writableBytes = bufferSize - bufferIndex;
}
+incrementWriteOps();
+ }
+
+ /**
+ * Increment Write Operations.
+ */
+ public void incrementWriteOps() {
+statistics.incrementWriteOps(1);
Review comment:
1. skip if statistics == null
2. make private
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392958021
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java
##
@@ -0,0 +1,111 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Stream.
+ */
+
+public class ITestAbfsStreamStatistics extends AbstractAbfsIntegrationTest {
+ public ITestAbfsStreamStatistics() throws Exception {
+ }
+
+ /***
+ * {@link AbfsInputStream#incrementReadOps()}.
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testAbfsStreamOps() throws Exception {
+describe("Test to see correct population of read and write operations in "
++ "Abfs");
+
+final AzureBlobFileSystem fs = getFileSystem();
+Path smallOperationsFile = new Path("testOneReadWriteOps");
+Path largeOperationsFile = new Path("testLargeReadWriteOps");
+FileSystem.Statistics statistics = fs.getFsStatistics();
+String testReadWriteOps = "test this";
+statistics.reset();
+
+//Test for zero read and write operation
+Assert.assertEquals("Mismatch in read operations", 0,
+statistics.getReadOps());
+Assert.assertEquals("Mismatch in write operations", 0,
+statistics.getWriteOps());
+
+FSDataOutputStream outForOneOperation = fs.create(smallOperationsFile);
Review comment:
close in a try/finally
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392957716
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java
##
@@ -0,0 +1,111 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Stream.
+ */
+
+public class ITestAbfsStreamStatistics extends AbstractAbfsIntegrationTest {
+ public ITestAbfsStreamStatistics() throws Exception {
+ }
+
+ /***
+ * {@link AbfsInputStream#incrementReadOps()}.
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testAbfsStreamOps() throws Exception {
+describe("Test to see correct population of read and write operations in "
++ "Abfs");
+
+final AzureBlobFileSystem fs = getFileSystem();
+Path smallOperationsFile = new Path("testOneReadWriteOps");
+Path largeOperationsFile = new Path("testLargeReadWriteOps");
+FileSystem.Statistics statistics = fs.getFsStatistics();
+String testReadWriteOps = "test this";
+statistics.reset();
+
+//Test for zero read and write operation
+Assert.assertEquals("Mismatch in read operations", 0,
+statistics.getReadOps());
+Assert.assertEquals("Mismatch in write operations", 0,
+statistics.getWriteOps());
+
+FSDataOutputStream outForOneOperation = fs.create(smallOperationsFile);
+statistics.reset();
+outForOneOperation.write(testReadWriteOps.getBytes());
+FSDataInputStream inForOneCall = fs.open(smallOperationsFile);
+inForOneCall.read(testReadWriteOps.getBytes(), 0,
+testReadWriteOps.getBytes().length);
+
+//Test for one read and write operation
Review comment:
factor into assertReadWriteOps(operation, stats, read, write)
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392958482
##
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java
##
@@ -252,6 +255,13 @@ int readRemote(long position, byte[] b, int offset, int
length) throws IOExcepti
return (int) bytesRead;
}
+ /**
+ * Increment Read Operations.
+ */
+ public void incrementReadOps() {
+statistics.incrementReadOps(1);
Review comment:
skip if statistics == 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392957031
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java
##
@@ -0,0 +1,111 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Stream.
+ */
+
+public class ITestAbfsStreamStatistics extends AbstractAbfsIntegrationTest {
+ public ITestAbfsStreamStatistics() throws Exception {
+ }
+
+ /***
+ * {@link AbfsInputStream#incrementReadOps()}.
Review comment:
just makes @code
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS URL: https://github.com/apache/hadoop/pull/1881#discussion_r392963453 ## File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java ## @@ -36,6 +36,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import org.apache.hadoop.fs.FileSystem.Statistics; Review comment: nit: ordering 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r392956693
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java
##
@@ -67,4 +77,46 @@ public void nameThread() {
protected int getTestTimeoutMillis() {
return TEST_TIMEOUT;
}
+
+ /**
+ * Describe a test in the logs.
+ *
+ * @param text text to print
+ * @param args arguments to format in the printing
+ */
+ protected void describe(String text, Object... args) {
+LOG.info("\n\n{}: {}\n",
+methodName.getMethodName(),
+String.format(text, args));
+ }
+
+ /**
+ * Validate Contents written on a file in Abfs.
+ *
+ * @param fsAzureBlobFileSystem
+ * @param path Path of the file
+ * @param originalByteArray original byte array
+ * @return
Review comment:
nit: Say what you return
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388858948
##
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
##
@@ -392,7 +392,7 @@ public void deleteFilesystem() throws
AzureBlobFileSystemException {
}
}
- public OutputStream createFile(final Path path, final boolean overwrite,
final FsPermission permission,
+ public OutputStream createFile(final Path path, final FileSystem.Statistics
statistics, final boolean overwrite, final FsPermission permission,
Review comment:
nit: this line is long enough its time to split
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388858679
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemOauth.java
##
@@ -143,7 +143,7 @@ public void testBlobDataReader() throws Exception {
// TEST WRITE FILE
try {
- abfsStore.openFileForWrite(EXISTED_FILE_PATH, true);
+ abfsStore.openFileForWrite(EXISTED_FILE_PATH, fs.getFsStatistics(),true);
Review comment:
nit, add a space after the comma
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS URL: https://github.com/apache/hadoop/pull/1881#discussion_r388854876 ## File path: hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java ## @@ -17,12 +17,19 @@ */ package org.apache.hadoop.fs.azurebfs; +import org.apache.hadoop.fs.FSDataInputStream; Review comment: nit: should be in its own block after the non org-apache-hadoop imports 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388856604
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsInputStream.java
##
@@ -0,0 +1,81 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Input Stream.
+ */
+
+public class ITestAbfsInputStream extends AbstractAbfsIntegrationTest {
+ public ITestAbfsInputStream() throws Exception {
+ }
+
+ /***
+ * {@link AbfsInputStream#incrementReadOps()}
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testAbfsInputStreamReadOps() throws Exception {
+describe("Test to see correct population of Read operations in Abfs");
+
+final AzureBlobFileSystem fs = getFileSystem();
+Path smallFile = new Path("testOneReadCall");
+Path largeFile = new Path("testLargeReadCalls");
+FileSystem.Statistics statistics = fs.getFsStatistics();
+String testReadOps = "test this";
+statistics.reset();
+
+//Test for zero read operation
+Assert.assertEquals(0, statistics.getReadOps());
+
+FSDataOutputStream outForOneCall = fs.create(smallFile);
+statistics.reset();
+outForOneCall.write(testReadOps.getBytes());
Review comment:
add a test for writes here too
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388858762
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsInputStream.java
##
@@ -0,0 +1,81 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Input Stream.
+ */
+
+public class ITestAbfsInputStream extends AbstractAbfsIntegrationTest {
Review comment:
This is a good test.
If you call it ITestAbfsStreamStatistics you can test the writes as well as
the reads -and you should test those writes already in the test suite.. If you
put the write ops test there, into a single test suite, backporting is easier
-less conflict.
For the assertions, I'd like every assert to have some meaningful string,
assertEquals("read ops", 1000, statistics.getReadOps)
Imagine "what would you want in the error text if this failed on a jenkins
run"?
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388859651
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java
##
@@ -59,6 +62,59 @@ public void testEnsureFileCreatedImmediately() throws
Exception {
assertIsFile(fs, TEST_FILE_PATH);
}
+ /**
+ * {@link AbfsOutputStream#incrementWriteOps()}
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testWriteOpsMetric() throws Exception {
Review comment:
move into the same tests as the reads for ease of backports
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388857077
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsInputStream.java
##
@@ -0,0 +1,81 @@
+/**
+ * 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.azurebfs;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.azurebfs.services.AbfsInputStream;
+
+/**
+ * Test Abfs Input Stream.
+ */
+
+public class ITestAbfsInputStream extends AbstractAbfsIntegrationTest {
+ public ITestAbfsInputStream() throws Exception {
+ }
+
+ /***
+ * {@link AbfsInputStream#incrementReadOps()}
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testAbfsInputStreamReadOps() throws Exception {
+describe("Test to see correct population of Read operations in Abfs");
+
+final AzureBlobFileSystem fs = getFileSystem();
+Path smallFile = new Path("testOneReadCall");
+Path largeFile = new Path("testLargeReadCalls");
+FileSystem.Statistics statistics = fs.getFsStatistics();
+String testReadOps = "test this";
+statistics.reset();
+
+//Test for zero read operation
+Assert.assertEquals(0, statistics.getReadOps());
+
+FSDataOutputStream outForOneCall = fs.create(smallFile);
+statistics.reset();
+outForOneCall.write(testReadOps.getBytes());
+FSDataInputStream inForOneCall = fs.open(smallFile);
+inForOneCall.read(testReadOps.getBytes(), 0,
testReadOps.getBytes().length);
+
+//Test for one read operation
+Assert.assertEquals(1, statistics.getReadOps());
+
+FSDataOutputStream outForLargeCalls = fs.create(largeFile);
Review comment:
does this really create large reads? it's still only small amounts of data
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS URL: https://github.com/apache/hadoop/pull/1881#discussion_r388854742 ## File path: hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java ## @@ -17,12 +17,19 @@ */ package org.apache.hadoop.fs.azurebfs; +import org.apache.hadoop.fs.FSDataInputStream; +import org.apache.hadoop.fs.Path; import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Rule; import org.junit.rules.TestName; import org.junit.rules.Timeout; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; Review comment: nit: topmost import block 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding file system counters in ABFS URL: https://github.com/apache/hadoop/pull/1881#discussion_r388855498 ## File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java ## @@ -252,6 +255,13 @@ int readRemote(long position, byte[] b, int offset, int length) throws IOExcepti return (int) bytesRead; } + /** + * Increment Read Operations Review comment: nit: trailing . 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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388855031
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java
##
@@ -67,4 +77,42 @@ public void nameThread() {
protected int getTestTimeoutMillis() {
return TEST_TIMEOUT;
}
+
+ /**
+ * Describe a test in the logs.
+ *
+ * @param text text to print
+ * @param args arguments to format in the printing
+ */
+ protected void describe(String text, Object... args) {
+LOG.info("\n\n{}: {}\n",
+methodName.getMethodName(),
+String.format(text, args));
+ }
+
+ /**
+ * Validate Contents written on a file in Abfs
Review comment:
nit: add a trailing . to keep javadoc happy
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 #1881: HADOOP-16910 Adding file system counters in ABFS
steveloughran commented on a change in pull request #1881: HADOOP-16910 Adding
file system counters in ABFS
URL: https://github.com/apache/hadoop/pull/1881#discussion_r388856133
##
File path:
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsTestWithTimeout.java
##
@@ -67,4 +77,42 @@ public void nameThread() {
protected int getTestTimeoutMillis() {
return TEST_TIMEOUT;
}
+
+ /**
+ * Describe a test in the logs.
+ *
+ * @param text text to print
+ * @param args arguments to format in the printing
+ */
+ protected void describe(String text, Object... args) {
+LOG.info("\n\n{}: {}\n",
+methodName.getMethodName(),
+String.format(text, args));
+ }
+
+ /**
+ * Validate Contents written on a file in Abfs
+ *
+ * @param fs AzureBlobFileSystem
+ * @param path Path of the file
+ * @param originalByteArray original byte array
+ * @return
+ * @throws IOException
+ */
+ protected boolean validateContent(AzureBlobFileSystem fs, Path path,
+ byte[] originalByteArray)
+ throws IOException {
+FSDataInputStream in = fs.open(path);
+byte[] contentByteArray = new byte[originalByteArray.length];
+int seekPos = 0;
+while (in.read() != -1) {
Review comment:
read() always moves the stream forward, so you don't need these
seek/seekPos++; they can only slow things 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.
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
