anujmodi2021 commented on code in PR #7272:
URL: https://github.com/apache/hadoop/pull/7272#discussion_r1903914295
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java:
##########
@@ -145,6 +145,11 @@ public final class FileSystemConfigurations {
*/
public static final int BLOCK_UPLOAD_ACTIVE_BLOCKS_DEFAULT = 20;
+ /**
+ * Length of the block ID used for appends.
+ */
+ public static final int BLOCK_ID_LENGTH = 60;
Review Comment:
Do we need this to be configurable with default/max 60?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/extensions/SASTokenProvider.java:
##########
@@ -41,6 +41,7 @@ public interface SASTokenProvider {
String GET_STATUS_OPERATION = "get-status";
String GET_PROPERTIES_OPERATION = "get-properties";
String LIST_OPERATION = "list";
+ String LIST_BLOB_OPERATION = "list-blob";
Review Comment:
These will only be needed if we support User Delegation SAS for FNS -Blob
right?
Let's discuss this once if they are needed for now.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -634,8 +869,8 @@ public AbfsRestOperation flush(final String path,
final String leaseId,
final ContextEncryptionAdapter contextEncryptionAdapter,
final TracingContext tracingContext) throws AzureBlobFileSystemException
{
- throw new UnsupportedOperationException(
- "Flush without blockIds not supported on Blob Endpoint");
+ return this.flush(null, path, isClose, cachedSasToken, leaseId, null,
contextEncryptionAdapter,
Review Comment:
This should remain Unsupported in my opinion.
Anyway passing buffer as null will fail (may be with NPE).
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlockStatus.java:
##########
@@ -0,0 +1,25 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.services;
+
Review Comment:
Java doc for class and enum values
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java:
##########
@@ -177,12 +197,180 @@ public AbfsOutputStream(AbfsOutputStreamContext
abfsOutputStreamContext)
this.tracingContext.setOperation(FSOperationType.WRITE);
this.ioStatistics = outputStreamStatistics.getIOStatistics();
this.blockFactory = abfsOutputStreamContext.getBlockFactory();
- this.blockSize = bufferSize;
- // create that first block. This guarantees that an open + close sequence
- // writes a 0-byte entry.
- createBlockIfNeeded();
+ this.isDFSToBlobFallbackEnabled
+ = abfsOutputStreamContext.isDFSToBlobFallbackEnabled();
+ this.serviceTypeAtInit = this.currentExecutingServiceType =
+ abfsOutputStreamContext.getIngressServiceType();
+ this.clientHandler = abfsOutputStreamContext.getClientHandler();
+ createIngressHandler(serviceTypeAtInit,
+ abfsOutputStreamContext.getBlockFactory(), bufferSize, false, null);
+ }
+
+ /**
+ * Retrieves the current ingress handler.
+ *
+ * @return the current {@link AzureIngressHandler}.
+ */
+ public AzureIngressHandler getIngressHandler() {
+ return ingressHandler;
+ }
+
+ private final Lock lock = new ReentrantLock();
+
+ private volatile boolean switchCompleted = false;
+
+ /**
+ * Creates or retrieves an existing Azure ingress handler based on the
service type and provided parameters.
+ * <p>
Review Comment:
seems like xml tag is not closed
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java:
##########
@@ -226,6 +414,9 @@ public void write(final int byteVal) throws IOException {
@Override
public synchronized void write(final byte[] data, final int off, final int
length)
throws IOException {
+ if (closed) {
Review Comment:
Why is this needed now?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -540,7 +729,6 @@ public AbfsRestOperation append(final String path,
addEncryptionKeyRequestHeaders(path, requestHeaders, false,
contextEncryptionAdapter, tracingContext);
requestHeaders.add(new AbfsHttpHeader(CONTENT_LENGTH,
String.valueOf(buffer.length)));
- requestHeaders.add(new AbfsHttpHeader(IF_MATCH, reqParams.getETag()));
Review Comment:
Why is this removed?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -508,8 +698,7 @@ public AbfsClientRenameResult renamePath(final String
source,
final String continuation,
final TracingContext tracingContext,
final String sourceEtag,
- final boolean isMetadataIncompleteState,
- final boolean isNamespaceEnabled) throws IOException {
Review Comment:
This must also be taken as part og Rename-Delete PR. We can remove this
change from this PR maybe
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##########
@@ -249,6 +251,27 @@ public static ApiVersion getCurrentVersion() {
*/
public static final Integer HTTP_STATUS_CATEGORY_QUOTIENT = 100;
+ /**
Review Comment:
In javadoc, please add `{@value}` for these constants
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##########
@@ -839,6 +839,11 @@ public boolean isSmallWriteOptimizationEnabled() {
return this.enableSmallWriteOptimization;
}
+ public void setSmallWriteOptimization(final boolean
enableSmallWriteOptimization) {
Review Comment:
Is it used somewhere? Else we can remove it
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/exceptions/InvalidIngressServiceException.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.contracts.exceptions;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+
[email protected]
[email protected]
+public final class InvalidIngressServiceException
Review Comment:
Javadoc for this class
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##########
@@ -1835,6 +1866,15 @@ private AbfsServiceType getAbfsServiceTypeFromUrl() {
return AbfsServiceType.DFS;
}
+ /**
+ * Retrieves the configured service type for the Azure Blob File System.
+ *
+ * @return the configured {@link AbfsServiceType} for the file system.
+ */
+ public AbfsServiceType getConfiguredServiceType() {
Review Comment:
Seems like not used, can be removed from this patch
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -756,7 +991,8 @@ public AbfsRestOperation setPathProperties(final String
path,
// This path could be present as an implicit directory in FNS.
Review Comment:
This might have added in my previous PR but line 983 is redundant and buggy,
can we remove that as part of this PR only. We should call op.execute() only
once.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -611,6 +799,53 @@ public AbfsRestOperation append(final String path,
return op;
}
+ /**
+ * Appends a block to an append blob.
+ * API reference: <a
href="https://learn.microsoft.com/en-us/rest/api/storageservices/append-block";></a>
Review Comment:
Can we follow the javadoc format as used in other APIs, we need to add API
details in blobEndpoint.md file and add link to that here.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -914,16 +1150,14 @@ public AbfsRestOperation read(final String path,
* @param recursive if the path is a directory, delete recursively.
* @param continuation to specify continuation token.
* @param tracingContext for tracing the server calls.
- * @param isNamespaceEnabled specify if the namespace is enabled.
* @return executed rest operation containing response from server.
* @throws AzureBlobFileSystemException if rest operation fails.
*/
@Override
public AbfsRestOperation deletePath(final String path,
final boolean recursive,
final String continuation,
- TracingContext tracingContext,
- final boolean isNamespaceEnabled) throws AzureBlobFileSystemException {
Review Comment:
Same here, this change could be taken in rename-delete PR
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlock.java:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.services;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+import org.apache.hadoop.fs.store.DataBlocks;
+
+/**
+ * Return activeBlock with blockId.
+ */
+public class AbfsBlock implements Closeable {
Review Comment:
Javadoc for all public methods here.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsErrors.java:
##########
@@ -43,11 +45,18 @@ public final class AbfsErrors {
public static final String ERR_LEASE_DID_NOT_MATCH = "The lease ID specified
did not match the "
+ "lease ID for the resource with the specified lease operation";
public static final String ERR_LEASE_BROKEN = "The lease ID matched, but the
lease has been "
- + "broken explicitly and cannot be renewed";
+ + "broken explicitly and cannot be renewed";
public static final String ERR_LEASE_FUTURE_EXISTS = "There is already an
existing lease "
+ "operation";
public static final String ERR_NO_LEASE_THREADS = "Lease desired but no
lease threads "
+ "configured, set " + FS_AZURE_LEASE_THREADS;
public static final String ERR_CREATE_ON_ROOT = "Cannot create file over
root path";
+ public static final String PATH_EXISTS = "The specified path, or an element
of the path, " +
+ "exists and its resource type is invalid for this operation.";
+ public static final String BLOB_OPERATION_NOT_SUPPORTED = "Blob operation is
not supported.";
+ public static final String INVALID_APPEND_OPERATION = "The resource was
created or modified by the Azure Blob Service API " +
+ "and cannot be appended to by the Azure Data Lake Storage Service API";
+ public static final String CONDITION_NOT_MET = "The condition specified
using " +
+ "HTTP conditional header(s) is not met.";
private AbfsErrors() {}
-}
+}
Review Comment:
EOF error
--
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]
