errose28 commented on code in PR #5144:
URL: https://github.com/apache/ozone/pull/5144#discussion_r1298944602
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java:
##########
@@ -388,6 +390,83 @@ public void testDeleteNonEmptyBucket() throws Exception {
}
}
+ @Test
+ public void testLinkedBucketResolution() throws Exception {
+ createSampleVol();
+ ECReplicationConfig ecConfig = new ECReplicationConfig(3, 2);
+ OmBucketInfo bucketInfo = OmBucketInfo.newBuilder()
+ .setVolumeName("sample-vol")
+ .setBucketName("bucket-one")
+ .setDefaultReplicationConfig(
+ new DefaultReplicationConfig(
+ ecConfig))
+ .build();
+ writeClient.createBucket(bucketInfo);
+
+ OmBucketInfo bucketLinkInfo = OmBucketInfo.newBuilder()
+ .setVolumeName("sample-vol")
+ .setBucketName("link-one")
+ .setSourceVolume("sample-vol")
+ .setSourceBucket("bucket-one")
+ .build();
+ writeClient.createBucket(bucketLinkInfo);
+
+ OmBucketInfo bucketLink2 = OmBucketInfo.newBuilder()
+ .setVolumeName("sample-vol")
+ .setBucketName("link-two")
+ .setSourceVolume("sample-vol")
+ .setSourceBucket("link-one")
+ .build();
+ writeClient.createBucket(bucketLink2);
+
+ OmBucketInfo storedLinkBucket =
+ writeClient.getBucketInfo("sample-vol", "link-two");
+ Assert.assertNotNull("Replication config is not set",
+ storedLinkBucket.getDefaultReplicationConfig());
+ Assert.assertEquals(ecConfig,
+ storedLinkBucket
+ .getDefaultReplicationConfig()
+ .getReplicationConfig());
+
+ Assert.assertEquals(
+ "link-two", storedLinkBucket.getBucketName());
+ Assert.assertEquals(
+ "sample-vol", storedLinkBucket.getVolumeName());
+
+ Assert.assertEquals(
+ "link-one", storedLinkBucket.getSourceBucket());
+ Assert.assertEquals(
+ "sample-vol", storedLinkBucket.getSourceVolume());
+
+ Assert.assertEquals(
+ bucketInfo.getBucketLayout(),
+ storedLinkBucket.getBucketLayout());
+ Assert.assertEquals(
+ bucketInfo.getQuotaInBytes(),
+ storedLinkBucket.getQuotaInBytes());
+ Assert.assertEquals(
+ bucketInfo.getQuotaInNamespace(),
+ storedLinkBucket.getQuotaInNamespace());
+ Assert.assertEquals(
+ bucketInfo.getUsedBytes(),
+ storedLinkBucket.getUsedBytes());
+ Assert.assertEquals(
+ bucketInfo.getUsedNamespace(),
+ storedLinkBucket.getUsedNamespace());
Review Comment:
We should set non-default values for these fields in the source bucket to
make sure the test is correctly resolving them, and we aren't getting a
coincidental match because the source and link are both reading the same
default. As implemented right now, the quota checks will pass on the master
branch for this reason, even though there is a bug there.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerUtils.java:
##########
@@ -62,12 +62,41 @@ private OzoneManagerUtils() {
* omMetadataManager().getBucketTable().get(buckKey)
*/
+ public static OmBucketInfo getBucketInfo(OMMetadataManager metaMgr,
+ String volName,
+ String buckName)
+ throws IOException {
+ OmBucketInfo bucketInfo = getOmBucketInfo(metaMgr, volName, buckName);
+ if (bucketInfo == null) {
+ reportNotFound(metaMgr, volName, buckName);
+ }
+ return bucketInfo;
+ }
+
private static OmBucketInfo getOmBucketInfo(OMMetadataManager metaMgr,
Review Comment:
Can we absorb this into `getBucketInfo`? That is its only caller and the
similar method names are confusing.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/BucketManagerImpl.java:
##########
@@ -69,27 +67,9 @@ public OmBucketInfo getBucketInfo(String volumeName, String
bucketName)
metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
bucketName);
try {
- String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
- OmBucketInfo value = metadataManager.getBucketTable().get(bucketKey);
- if (value == null) {
- LOG.debug("bucket: {} not found in volume: {}.", bucketName,
- volumeName);
- // Check parent volume existence
- final String dbVolumeKey = metadataManager.getVolumeKey(volumeName);
- if (metadataManager.getVolumeTable().get(dbVolumeKey) == null) {
- // Parent volume doesn't exist, throw VOLUME_NOT_FOUND
- throw new OMException("Volume not found when getting bucket info",
- VOLUME_NOT_FOUND);
- } else {
- // Parent volume exists, throw BUCKET_NOT_FOUND
- throw new OMException("Bucket not found", BUCKET_NOT_FOUND);
- }
- }
-
- value = OzoneManagerUtils.resolveLinkBucketLayout(value, metadataManager,
- new HashSet<>());
-
- return value;
+ //this is a read operation, does not resolve bucket link
+ return OzoneManagerUtils.getBucketInfo(metadataManager,
+ volumeName, bucketName);
Review Comment:
I'm not sure what this comment means. This method is called by
`OzoneManager#getBucketInfo` which is called by write operations like
`setBucketQuota` and `rename`. Even if it was a read only operation, we would
still want to resolve the properties like quota and bucket layout from the
source bucket.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/PrefixParser.java:
##########
@@ -147,17 +147,16 @@ public void parse(String vol, String buck, String db,
parserStats[Types.VOLUME.ordinal()]++;
// First get the info about the bucket
- String bucketKey = metadataManager.getBucketKey(vol, buck);
- OmBucketInfo info = metadataManager.getBucketTable().get(bucketKey);
- if (info == null) {
+ OmBucketInfo info;
+ try {
+ info = OzoneManagerUtils
+ .getResolvedBucketInfo(metadataManager, vol, buck, true);
Review Comment:
Why should the prefix parser allow dangling links? If there is no source
bucket, we cannot know the bucket layout.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerUtils.java:
##########
@@ -81,122 +110,79 @@ public static BucketLayout
getBucketLayout(OMMetadataManager metadataManager,
String volName,
String buckName)
throws IOException {
- return getBucketLayout(metadataManager, volName, buckName, new
HashSet<>());
+ return getResolvedBucketInfo(metadataManager, volName, buckName, false)
+ .getBucketLayout();
}
/**
- * Get bucket layout for the given volume and bucket name.
+ * Get bucket info for the given volume and bucket name.
*
* @param metadataManager metadata manager
* @param volName volume name
* @param buckName bucket name
- * @return bucket layout
+ * @return bucket info
* @throws IOException
*/
- private static BucketLayout getBucketLayout(OMMetadataManager
metadataManager,
- String volName,
- String buckName,
- Set<Pair<String, String>>
visited)
+ public static OmBucketInfo getResolvedBucketInfo(
+ OMMetadataManager metadataManager,
+ String volName,
+ String buckName,
+ boolean allowDanglingBuckets)
throws IOException {
-
- OmBucketInfo buckInfo = getOmBucketInfo(metadataManager, volName,
buckName);
-
- if (buckInfo != null) {
- // If this is a link bucket, we fetch the BucketLayout from the
- // source bucket.
- if (buckInfo.isLink()) {
- // Check if this bucket was already visited - to avoid loops
- if (!visited.add(Pair.of(volName, buckName))) {
- throw new OMException("Detected loop in bucket links. Bucket name: "
+
- buckName + ", Volume name: " + volName,
- DETECTED_LOOP_IN_BUCKET_LINKS);
- }
- OmBucketInfo sourceBuckInfo =
- getOmBucketInfo(metadataManager, buckInfo.getSourceVolume(),
- buckInfo.getSourceBucket());
- if (sourceBuckInfo != null) {
- /** If the source bucket is again a link, we recursively resolve the
- * link bucket.
- *
- * For example:
- * buck-link1 -> buck-link2 -> buck-link3 -> buck-src
- * buck-src has the actual BucketLayout that will be used by the
- * links.
- */
- if (sourceBuckInfo.isLink()) {
- return getBucketLayout(metadataManager,
- sourceBuckInfo.getVolumeName(),
- sourceBuckInfo.getBucketName(), visited);
- }
- return sourceBuckInfo.getBucketLayout();
- }
- }
- return buckInfo.getBucketLayout();
- }
-
- if (!metadataManager.getVolumeTable()
- .isExist(metadataManager.getVolumeKey(volName))) {
- throw new OMException("Volume not found: " + volName,
- OMException.ResultCodes.VOLUME_NOT_FOUND);
- }
-
- throw new OMException("Bucket not found: " + volName + "/" + buckName,
- OMException.ResultCodes.BUCKET_NOT_FOUND);
+ return resolveBucketInfoLink(metadataManager, volName, buckName,
+ new HashSet<>(), allowDanglingBuckets);
}
/**
- * Resolve bucket layout for a given link bucket's OmBucketInfo.
+ * Get bucket info for the given volume and bucket name.
*
- * @param bucketInfo
- * @return {@code OmBucketInfo} with
+ * @param metadataManager metadata manager
+ * @param volName volume name
+ * @param buckName bucket name
+ * @return bucket info
* @throws IOException
*/
- public static OmBucketInfo resolveLinkBucketLayout(OmBucketInfo bucketInfo,
- OMMetadataManager
- metadataManager,
- Set<Pair<String,
- String>> visited)
+ private static OmBucketInfo resolveBucketInfoLink(
+ OMMetadataManager metadataManager,
+ String volName,
+ String buckName,
+ Set<Pair<String, String>> visited,
+ boolean allowDanglingBuckets)
Review Comment:
can we get rid of this parameter? I don't see any uses where the dangling
bucket link gives the correct result.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerUtils.java:
##########
@@ -81,122 +110,79 @@ public static BucketLayout
getBucketLayout(OMMetadataManager metadataManager,
String volName,
String buckName)
throws IOException {
- return getBucketLayout(metadataManager, volName, buckName, new
HashSet<>());
+ return getResolvedBucketInfo(metadataManager, volName, buckName, false)
+ .getBucketLayout();
}
/**
- * Get bucket layout for the given volume and bucket name.
+ * Get bucket info for the given volume and bucket name.
*
* @param metadataManager metadata manager
* @param volName volume name
* @param buckName bucket name
- * @return bucket layout
+ * @return bucket info
* @throws IOException
*/
- private static BucketLayout getBucketLayout(OMMetadataManager
metadataManager,
- String volName,
- String buckName,
- Set<Pair<String, String>>
visited)
+ public static OmBucketInfo getResolvedBucketInfo(
+ OMMetadataManager metadataManager,
+ String volName,
+ String buckName,
+ boolean allowDanglingBuckets)
throws IOException {
-
- OmBucketInfo buckInfo = getOmBucketInfo(metadataManager, volName,
buckName);
-
- if (buckInfo != null) {
- // If this is a link bucket, we fetch the BucketLayout from the
- // source bucket.
- if (buckInfo.isLink()) {
- // Check if this bucket was already visited - to avoid loops
- if (!visited.add(Pair.of(volName, buckName))) {
- throw new OMException("Detected loop in bucket links. Bucket name: "
+
- buckName + ", Volume name: " + volName,
- DETECTED_LOOP_IN_BUCKET_LINKS);
- }
- OmBucketInfo sourceBuckInfo =
- getOmBucketInfo(metadataManager, buckInfo.getSourceVolume(),
- buckInfo.getSourceBucket());
- if (sourceBuckInfo != null) {
- /** If the source bucket is again a link, we recursively resolve the
- * link bucket.
- *
- * For example:
- * buck-link1 -> buck-link2 -> buck-link3 -> buck-src
- * buck-src has the actual BucketLayout that will be used by the
- * links.
- */
- if (sourceBuckInfo.isLink()) {
- return getBucketLayout(metadataManager,
- sourceBuckInfo.getVolumeName(),
- sourceBuckInfo.getBucketName(), visited);
- }
- return sourceBuckInfo.getBucketLayout();
- }
- }
- return buckInfo.getBucketLayout();
- }
-
- if (!metadataManager.getVolumeTable()
- .isExist(metadataManager.getVolumeKey(volName))) {
- throw new OMException("Volume not found: " + volName,
- OMException.ResultCodes.VOLUME_NOT_FOUND);
- }
-
- throw new OMException("Bucket not found: " + volName + "/" + buckName,
- OMException.ResultCodes.BUCKET_NOT_FOUND);
+ return resolveBucketInfoLink(metadataManager, volName, buckName,
+ new HashSet<>(), allowDanglingBuckets);
}
/**
- * Resolve bucket layout for a given link bucket's OmBucketInfo.
+ * Get bucket info for the given volume and bucket name.
*
- * @param bucketInfo
- * @return {@code OmBucketInfo} with
+ * @param metadataManager metadata manager
+ * @param volName volume name
+ * @param buckName bucket name
+ * @return bucket info
Review Comment:
Can you update this comment to specify what gets returned if the link is or
is not pointing to a valid source bucket?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -2775,7 +2775,32 @@ public OmBucketInfo getBucketInfo(String volume, String
bucket)
bucket, null);
}
metrics.incNumBucketInfos();
- return bucketManager.getBucketInfo(volume, bucket);
+
+ ResolvedBucket resolvedBucket =
+ resolveBucketLink(Pair.of(volume, bucket), true);
Review Comment:
Similar comment to other places, I think we should fail the request if the
source bucket does not exist. For example, `OMFileCreateRequest` is calling
this method to get the replication config. Replication config should be taken
from the source bucket, so this will not work correctly if the dangling link is
returned.
--
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]
