ayushtkn commented on code in PR #10907:
URL: https://github.com/apache/ozone/pull/10907#discussion_r3688042210
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java:
##########
@@ -237,19 +260,20 @@ private static Striped<ReentrantReadWriteLock>
createStripeLock(Resource r, Conf
return SimpleStriped.readWriteLock(size, fair);
}
- private Iterable<ReentrantReadWriteLock>
getAllLocks(Striped<ReentrantReadWriteLock> striped) {
- return IntStream.range(0,
striped.size()).mapToObj(striped::getAt).collect(Collectors.toList());
- }
+ static List<ReentrantReadWriteLock> bulkGet(Striped<ReentrantReadWriteLock>
striped, Iterable<String[]> keys) {
+ final Iterable<ReentrantReadWriteLock> iterable = striped.bulkGet(
+ CollectionUtils.as(keys, CompositeKey::combineKeys)); // no copying
+ // although the return type of Striped.bulkGet(..) is Iterable, its
implementation currently returns a List.
+ if (iterable instanceof List) {
+ return (List<ReentrantReadWriteLock>) iterable;
+ }
- private Iterable<ReentrantReadWriteLock>
bulkGetLock(Striped<ReentrantReadWriteLock> striped,
- Collection<String[]> keys) {
- List<Object> lockKeys = new ArrayList<>(keys.size());
- for (String[] key : keys) {
- if (Objects.nonNull(key)) {
- lockKeys.add(CompositeKey.combineKeys(key));
- }
+ // fallback copying to a list
+ final List<ReentrantReadWriteLock> list = new LinkedList<>();
Review Comment:
Curious why did you choose `LinkedList` here, to avoid resizing while
inserting or so? Looking at one of the caller
```
final List<ReentrantReadWriteLock> list = bulkGet(lockMap.get(r),
keys);
// Release locks in reverse order.
for (int i = list.size() - 1; i >= 0; i--) {
releaseLock(r, isRead, list.get(i));
}
```
If the returned type is `LinkedList` this `list.get(i)` would be `O(n)` vs
`O(1)` if you would have chosen `ArrayList`, so overall `O(n^2)`
I don't think `ArrayList` would be costly here, it has to occasionally
resize only. But if you want to use `LinkedList` maybe in the caller you can
use `ListIterator` with `.previous()`, that would drop the `O(n^2)` to `O(n)`
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotMoveTableKeysResponse.java:
##########
@@ -128,9 +128,9 @@ public void testMoveTableKeysToNextSnapshot(boolean
nextSnapshotExists) throws E
getVolumeName(), getBucketName(), snapshotName1);
UncheckedAutoCloseableSupplier<OmSnapshot> snapshot2 =
nextSnapshotExists ? getOmSnapshotManager().getSnapshot(
getVolumeName(), getBucketName(), snapshotName2) : null) {
- List<List<String>> expectedSnapshotIdLocks =
-
Arrays.asList(Collections.singletonList(snapshot1.get().getSnapshotID().toString()),
- nextSnapshotExists ?
Collections.singletonList(snapshot2.get().getSnapshotID().toString()) : null);
+ final List<String> first =
Collections.singletonList(snapshot1.get().getSnapshotID().toString());
Review Comment:
nit: there double space after `=`
--
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]
