Skip to content

Changes done for adding formats to the Azure Blob Storage plugin#163

Open
vikasrathee-cs wants to merge 1 commit into
data-integrations:developfrom
cloudsufi:guava-cdap-ui-fix
Open

Changes done for adding formats to the Azure Blob Storage plugin#163
vikasrathee-cs wants to merge 1 commit into
data-integrations:developfrom
cloudsufi:guava-cdap-ui-fix

Conversation

@vikasrathee-cs

Copy link
Copy Markdown

Changes done for adding formats to the Azure Blob Storage plugin

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Azure Blob Store batch source to support Azure Data Lake Storage Gen2 (ABFS) alongside WASB, introducing new authentication methods such as Service Principal and Managed Identity. The implementation is updated to extend AbstractFileSource instead of AbstractFileBatchSource, with corresponding updates to documentation, dependencies, and UI widgets. The review feedback highlights critical safety improvements, specifically pointing out potential NullPointerException risks when validating or parsing null path and account values, as well as identifying obsolete UI widgets that should be removed.

Comment thread azure-blob-store/widgets/AzureBlobStore-batchsource.json Outdated
Comment thread pom.xml
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<cdap.version>6.11.0</cdap.version>
<cdap.version>6.10.0</cdap.version>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the downgrade ?


@Override
protected void recordLineage(LineageRecorder lineageRecorder, List<String> outputFields) {
lineageRecorder.recordRead("Read", "Read from Azure Blob Storage.", outputFields);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit :Change to "Read from Azure Storage" .

So, it covers gen 2 as well.

.withConfigProperty(PATH);

boolean isWasb = !containsMacro(NAME_PATH) &&
path != null && (path.startsWith("wasb://") || path.startsWith("wasbs://"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create private static final String constants at top for wasb://, wasbs:// etc... and replace these.

Applicable for all below.

}

String authMethod = config.authenticationMethod;
String account = config.account;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can account be null / macro ?

Comment thread pom.xml
<cdap.plugin.version>2.13.0</cdap.plugin.version>
<cdap.plugin.version>2.12.1</cdap.plugin.version>
<jackson.databind.version>2.13.4.2</jackson.databind.version>
<guava.version>27.0-jre</guava.version>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed ?
If so can we target the guava version we are going to use in CDAP in guava upgrade case ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants