adoroszlai commented on code in PR #7971:
URL: https://github.com/apache/ozone/pull/7971#discussion_r1971090329
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreWithFSO.java:
##########
@@ -562,48 +563,38 @@ private void checkKeyList(Iterator<? extends OzoneKey >
ozoneKeyIterator,
assertEquals(keys, outputKeys);
}
- private void createKeys(OzoneBucket ozoneBucket, List<String> keys)
+ private void createAndAssertKeys(OzoneBucket ozoneBucket, List<String> keys)
throws Exception {
- int length = 10;
- byte[] input = new byte[length];
- Arrays.fill(input, (byte) 96);
for (String key : keys) {
- createKey(ozoneBucket, key, 10, input);
+ int length = 10;
+ byte[] input = new byte[length];
+ Arrays.fill(input, (byte) 96);
Review Comment:
`input` has the same content for each key, no need to move it inside the
loop.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSWithObjectStoreCreate.java:
##########
@@ -413,16 +414,11 @@ private void checkKeyList(Iterator<? extends OzoneKey >
ozoneKeyIterator,
assertEquals(keys, outputKeys);
}
- private void createKey(OzoneBucket ozoneBucket, String key, int length,
- byte[] input)
+ private void createAndAssertKey(OzoneBucket ozoneBucket, String key, int
length,
+ byte[] input)
Review Comment:
nit: Please do not format method signature like this. Whenever visibility /
return type / method name / other modifiers are changed, we would have to
reindent parameters.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreWithFSO.java:
##########
@@ -515,12 +517,11 @@ public void testListKeysWithNotNormalizedPath() throws
Exception {
byte[] input = new byte[length];
Arrays.fill(input, (byte)96);
- createKey(ozoneBucket, key1, 10, input);
- createKey(ozoneBucket, key2, 10, input);
- createKey(ozoneBucket, key3, 10, input);
+ createAndAssertKeys(ozoneBucket, Collections.singletonList(key1));
+ createAndAssertKeys(ozoneBucket, Collections.singletonList(key2));
+ createAndAssertKeys(ozoneBucket, Collections.singletonList(key3));
Review Comment:
Since `createAndAssertKeys` accepts a list, we don't need to call it 3x with
a singleton list.
```suggestion
createAndAssertKeys(ozoneBucket, Arrays.asList(key1, key2, key3));
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestBucketOwner.java:
##########
@@ -118,7 +120,8 @@ public void testNonBucketNonVolumeOwner() throws Exception {
assertThrows(Exception.class, () -> {
OzoneVolume volume = client.getObjectStore().getVolume(VOLUME_NAME);
OzoneBucket ozoneBucket = volume.getBucket("bucket1");
- createKey(ozoneBucket, "key3", 10, new byte[10]);
+ byte[] bytes = new byte[10];
+ createKey(ozoneBucket, "key3", new String(bytes,
StandardCharsets.UTF_8));
Review Comment:
Byte array can be kept inline with the method call, no need for the local
variable. (In other places, too.)
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestListKeysWithFSO.java:
##########
@@ -641,33 +643,23 @@ private void checkKeyShallowList(String keyPrefix, String
startKey,
checkKeyList(keyPrefix, startKey, keys, fsoBucket, true);
}
- private static void createKeys(OzoneBucket ozoneBucket, List<String> keys)
+ private static void createAndAssertKeys(OzoneBucket ozoneBucket,
List<String> keys)
throws Exception {
int length = 10;
byte[] input = new byte[length];
Arrays.fill(input, (byte) 96);
for (String key : keys) {
- createKey(ozoneBucket, key, 10, input);
- }
- }
-
- private static void createKey(OzoneBucket ozoneBucket, String key, int
length,
- byte[] input) throws Exception {
+ createKey(ozoneBucket, key, ReplicationFactor.THREE,
+ ReplicationType.RATIS, new String(input, StandardCharsets.UTF_8));
- OzoneOutputStream ozoneOutputStream =
- ozoneBucket.createKey(key, length);
+ // Read the key with given key name.
+ OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
+ byte[] read = new byte[length];
+ ozoneInputStream.read(read, 0, length);
+ ozoneInputStream.close();
- ozoneOutputStream.write(input);
- ozoneOutputStream.write(input, 0, 10);
- ozoneOutputStream.close();
-
- // Read the key with given key name.
- OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
- byte[] read = new byte[length];
- ozoneInputStream.read(read, 0, length);
- ozoneInputStream.close();
-
- assertEquals(new String(input, StandardCharsets.UTF_8),
- new String(read, StandardCharsets.UTF_8));
+ assertEquals(new String(input, StandardCharsets.UTF_8),
+ new String(read, StandardCharsets.UTF_8));
+ }
Review Comment:
Let's keep this method as `readKey` without the write part.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreWithFSO.java:
##########
@@ -562,48 +563,38 @@ private void checkKeyList(Iterator<? extends OzoneKey >
ozoneKeyIterator,
assertEquals(keys, outputKeys);
}
- private void createKeys(OzoneBucket ozoneBucket, List<String> keys)
+ private void createAndAssertKeys(OzoneBucket ozoneBucket, List<String> keys)
throws Exception {
- int length = 10;
- byte[] input = new byte[length];
- Arrays.fill(input, (byte) 96);
for (String key : keys) {
- createKey(ozoneBucket, key, 10, input);
+ int length = 10;
+ byte[] input = new byte[length];
+ Arrays.fill(input, (byte) 96);
+
+ createKey(ozoneBucket, key, new String(input, StandardCharsets.UTF_8));
+
+ // Read the key with given key name.
+ OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
+ byte[] read = new byte[length];
+ ozoneInputStream.read(read, 0, length);
+ ozoneInputStream.close();
+
+ String inputString = new String(input, StandardCharsets.UTF_8);
+ assertEquals(inputString, new String(read, StandardCharsets.UTF_8));
+
+ // Read using filesystem.
+ String rootPath = String.format("%s://%s.%s/", OZONE_URI_SCHEME,
+ bucketName, volumeName, StandardCharsets.UTF_8);
+ OzoneFileSystem o3fs = (OzoneFileSystem) FileSystem.get(new
URI(rootPath),
+ conf);
+ FSDataInputStream fsDataInputStream = o3fs.open(new Path(key));
+ read = new byte[length];
+ fsDataInputStream.read(read, 0, length);
+ fsDataInputStream.close();
+
+ assertEquals(inputString, new String(read, StandardCharsets.UTF_8));
}
}
- private void createKey(OzoneBucket ozoneBucket, String key, int length,
- byte[] input) throws Exception {
-
- OzoneOutputStream ozoneOutputStream =
- ozoneBucket.createKey(key, length);
-
- ozoneOutputStream.write(input);
- ozoneOutputStream.write(input, 0, 10);
- ozoneOutputStream.close();
-
- // Read the key with given key name.
- OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
- byte[] read = new byte[length];
- ozoneInputStream.read(read, 0, length);
- ozoneInputStream.close();
-
- String inputString = new String(input, StandardCharsets.UTF_8);
- assertEquals(inputString, new String(read, StandardCharsets.UTF_8));
-
- // Read using filesystem.
- String rootPath = String.format("%s://%s.%s/", OZONE_URI_SCHEME,
- bucketName, volumeName, StandardCharsets.UTF_8);
- OzoneFileSystem o3fs = (OzoneFileSystem) FileSystem.get(new URI(rootPath),
- conf);
- FSDataInputStream fsDataInputStream = o3fs.open(new Path(key));
- read = new byte[length];
- fsDataInputStream.read(read, 0, length);
- fsDataInputStream.close();
-
- assertEquals(inputString, new String(read, StandardCharsets.UTF_8));
- }
-
Review Comment:
Let's keep this method as `readKey` without the write part. So the loop
would look like:
```java
for (String key : keys) {
createKey(...)
readKey(...)
}
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestListKeys.java:
##########
@@ -369,32 +371,22 @@ private void checkKeyShallowList(String keyPrefix, String
startKey,
assertEquals(keys, outputKeysList);
}
- private static void createKeys(OzoneBucket ozoneBucket, List<String> keys)
+ private static void createAndAssertKeys(OzoneBucket ozoneBucket,
List<String> keys)
throws Exception {
int length = 10;
byte[] input = new byte[length];
Arrays.fill(input, (byte) 96);
for (String key : keys) {
- createKey(ozoneBucket, key, 10, input);
- }
- }
-
- private static void createKey(OzoneBucket ozoneBucket, String key, int
length,
- byte[] input) throws Exception {
+ createKey(ozoneBucket, key, ReplicationFactor.THREE,
ReplicationType.RATIS,
+ new String(input, StandardCharsets.UTF_8));
- OzoneOutputStream ozoneOutputStream =
- ozoneBucket.createKey(key, length);
+ // Read the key with given key name.
+ OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
+ byte[] read = new byte[length];
+ ozoneInputStream.read(read, 0, length);
+ ozoneInputStream.close();
- ozoneOutputStream.write(input);
- ozoneOutputStream.write(input, 0, 10);
- ozoneOutputStream.close();
-
- // Read the key with given key name.
- OzoneInputStream ozoneInputStream = ozoneBucket.readKey(key);
- byte[] read = new byte[length];
- ozoneInputStream.read(read, 0, length);
- ozoneInputStream.close();
-
- assertEquals(new String(input, StandardCharsets.UTF_8), new String(read,
StandardCharsets.UTF_8));
+ assertEquals(new String(input, StandardCharsets.UTF_8), new String(read,
StandardCharsets.UTF_8));
+ }
Review Comment:
Let's keep this method as `readKey` without the write part. So the loop
would look like:
```java
for (String key : keys) {
createKey(...)
readKey(...)
}
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSWithObjectStoreCreate.java:
##########
@@ -413,16 +414,11 @@ private void checkKeyList(Iterator<? extends OzoneKey >
ozoneKeyIterator,
assertEquals(keys, outputKeys);
}
- private void createKey(OzoneBucket ozoneBucket, String key, int length,
- byte[] input)
+ private void createAndAssertKey(OzoneBucket ozoneBucket, String key, int
length,
+ byte[] input)
throws Exception {
-
- OzoneOutputStream ozoneOutputStream =
- ozoneBucket.createKey(key, length);
-
- ozoneOutputStream.write(input);
- ozoneOutputStream.write(input, 0, 10);
- ozoneOutputStream.close();
+
+ createKey(ozoneBucket, key, new String(input, UTF_8));
Review Comment:
Sorry I haven't noticed that we write `byte[]` but `createKey` accepts
`String`.
Instead of wrapping in `new String` in each call (here and the other test
classes), let's create a new version of `createKey` in `TestDataUtil` that
accepts `byte[]`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreWithFSO.java:
##########
@@ -515,12 +517,11 @@ public void testListKeysWithNotNormalizedPath() throws
Exception {
byte[] input = new byte[length];
Arrays.fill(input, (byte)96);
Review Comment:
`input` becomes unused by calling the method that creates its own input, so
it can be removed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
