Nio set default configurations #27340

rickle-msft · 2022-02-28T21:05:18Z

Resolves #23653

…ironment

This reverts commit f47c221.

…ironment

azure-sdk · 2022-02-28T21:22:34Z

API changes have been detected in com.azure:azure-storage-blob-nio. You can review API changes here

API changes

+         public static void setDefaultConfigurations(Map<String, Object> config)

kasobol-msft · 2022-02-28T21:16:20Z

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java


    // Specs require a public zero argument constructor.
    /**
     * Creates an AzureFileSystemProvider.
     */
    public AzureFileSystemProvider() {
        this.openFileSystems = new ConcurrentHashMap<>();
+        Configuration config = Configuration.getGlobalConfiguration();


Are there scenarios where user can call AzureFileSystemProvider ctor directly? If yes we could consider adding AzureFileSystemProvider(Configuration) overload in case somebody would like to customize configuration source.

Btw. what do we do with config ? it doesn't seem to be consumed anyhow.

This was left over from when the original request was to set defaults from the environment, but the customer ask has changed to setting them on a static method. I thought I had reverted all the commits about environment reading but it looks like I missed one.

But in general no, there customer does not generally use the constructor directly. The jvm's ClassLoader will find it an initialize it

kasobol-msft · 2022-02-28T21:22:23Z

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java

+     *
+     * @param config The configurations map. Please see the docs on {@link AzureFileSystemProvider for more information}
+     */
+    public static void setDefaultConfigurations(Map<String, Object> config) {


is there any alternative design where this doesn't have to be a global ?

I think the request was that it be global. What are your concerns around that? Perhaps I can find another way to address those and meet the customer ask.

There's plenty of literature why global variables are dangerous. In this particular case the concern would be that users can modify this in flight while application is in the middle of processing workload which can potentially open a can of interesting race conditions for them.

On the other hand I understand the need for this convenience given that it's not the user interacting with types in this package but NIO magic doing it for them.

It seems the scenario here is to be able programmatically configure this at the startup of the application. I suggest we implement it in such a way that this configuration can be set only once during lifetime of user's program, so that they won't be tempted to modifying those settings in-flight.

Makes sense. I can add an if statement to see if the values have already been set and throw if they have. I think that can satisfy the temptation to change things mid run, but if you are still more concerned about race conditions, I could try doing something with atomic values to prevent the chance of even very tight concurrent updates on startup. The latter strikes me as overkill personally

"Double checked lock" might be good candidate for that.

I'll look into that and put something up for your consideration.

kasobol-msft · 2022-02-28T21:22:58Z

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java

+     * @param config The configurations map. Please see the docs on {@link AzureFileSystemProvider for more information}
+     */
+    public static void setDefaultConfigurations(Map<String, Object> config) {
+        defaultConfigurations = Collections.unmodifiableMap(config);


this doesn't block somebody with reference to config from making further modifications. Should map be copied?

kasobol-msft · 2022-02-28T21:23:49Z

sdk/storage/azure-storage-blob-nio/CHANGELOG.md

@@ -3,6 +3,9 @@
 ## 12.0.0-beta.17 (Unreleased)

 ### Features Added
+- Added `AzurePath.fromBlobUrl` to help convert from a blob url to an AzurePath
+- Added a configuration option `AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK` to skip the initial container check in cases where the authentication method used will not have necessary permissions.


there doesn't seem to be a reference to this option in the PR.

Mishandled a merge with main here and this is a duplicate entry. Removing

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java

…rage/blob/nio/AzureFileSystemProvider.java Co-authored-by: Kamil Sobol <[email protected]>

kasobol-msft

lgtm

kasobol-msft · 2022-03-03T20:53:44Z

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java

+                if (defaultConfigurations == null) {
+                    defaultConfigurations = Collections.unmodifiableMap(new HashMap<>(config));
+                } else {
+                    throw new IllegalStateException("Default Configurations can be set only once");


please call this out in javadoc.

There should be a throws declaration on the javadoc

…azure-sdk-for-java into nioConfigureFromEnvironment

rickle-msft · 2022-03-03T22:22:14Z

/azp run java - storage - ci

azure-pipelines · 2022-03-03T22:22:33Z

Azure Pipelines successfully started running 1 pipeline(s).

check-enforcer · 2022-03-04T00:14:00Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

further api changes.

kasobol-msft · 2022-03-04T00:22:38Z

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java

+     * @param endpoint The default endpoint to set. May be null
+     * @throws IllegalStateException If the configurations have already been set
+     */
+    public static void setDefaultConfigurations(Map<String, Object> config, String endpoint) {


Looks like we extracted endpoint for convenience. Which seems to indicate that Map<String, Object> is not easy to use, i.e. it doesn't describe very well types and properties allowed.
Since this API isn't related to any NIO SPI then maybe we should consider taking here strongly typed options instead of map and convert it into Map<String,Object> behind the scenes?

Without making any claims about whether the map is easy to use or not as that can be a separate discussion, we extracted the endpoint for a different reason than for convenience. Typically, customers will create a new file system via FileSystemProvider#newFileSystem(uri, map). The idea was to imitate this signature but allow access to the behavior in a different location (though looking at it, I should potentially swap the order). The ask for a default endpoint came later in the discussion, and I didn't want to add it into the map because we don't respect that map option in the newFileSystem() method as that's not how it's intended to be used. It seemed like it would be confusing if the map options were identical except for the endpoint option which was respected in one method but ignored in another.

ghost · 2022-05-06T10:03:00Z

Hi @rickle-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

ghost · 2022-05-13T11:02:11Z

Hi @rickle-msft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

rickle-msft added 6 commits February 3, 2022 15:17

Started some outline work

616408c

Merge remote-tracking branch 'upstream/main' into nioConfigureFromEnv…

b33da73

…ironment

Fiddled around with some stuff

f47c221

Revert "Fiddled around with some stuff"

4d8eff7

This reverts commit f47c221.

Added tests and recordings and changelog

8c9b328

Merge remote-tracking branch 'upstream/main' into nioConfigureFromEnv…

43dd700

…ironment

rickle-msft requested review from amishra-dev, jaschrep-msft, alzimmermsft and kasobol-msft as code owners February 28, 2022 21:05

ghost added the Storage Storage Service (Queues, Blobs, Files) label Feb 28, 2022

kasobol-msft reviewed Feb 28, 2022

View reviewed changes

rickle-msft changed the title ~~Nio configure from environment~~ Nio set default configurations Feb 28, 2022

Cleanup

7e0ea23

rickle-msft requested review from srnagar, anuchandy, lmolkova, pallavit, vcolin7, mssfang, JonathanGiles and billwert as code owners March 3, 2022 19:26

kasobol-msft reviewed Mar 3, 2022

View reviewed changes

...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java Outdated Show resolved Hide resolved

Added double checked locking to setting global config

3fe5a8f

rickle-msft force-pushed the nioConfigureFromEnvironment branch from 949234e to 3fe5a8f Compare March 3, 2022 20:34

Update sdk/storage/azure-storage-blob-nio/src/main/java/com/azure/sto…

3ada070

…rage/blob/nio/AzureFileSystemProvider.java Co-authored-by: Kamil Sobol <[email protected]>

kasobol-msft previously approved these changes Mar 3, 2022

View reviewed changes

rickle-msft added 2 commits March 3, 2022 12:56

ci fixes

846786c

Merge branch 'nioConfigureFromEnvironment' of github.com:rickle-msft/…

2a69364

…azure-sdk-for-java into nioConfigureFromEnvironment

rickle-msft added 3 commits March 3, 2022 14:55

Ci experiment

bc6b394

Ci experiment

b358bbc

Started making some changes based on customer feedback

e605ecd

kasobol-msft reviewed Mar 4, 2022

View reviewed changes

ghost added the no-recent-activity There has been no recent activity on this issue. label May 6, 2022

ghost closed this May 13, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nio set default configurations #27340

Nio set default configurations #27340

rickle-msft commented Feb 28, 2022

azure-sdk commented Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft Feb 28, 2022

rickle-msft Feb 28, 2022

kasobol-msft left a comment

kasobol-msft Mar 3, 2022

rickle-msft Mar 3, 2022

rickle-msft commented Mar 3, 2022

azure-pipelines bot commented Mar 3, 2022

check-enforcer bot commented Mar 4, 2022

kasobol-msft Mar 4, 2022

rickle-msft Mar 4, 2022

ghost commented May 6, 2022

ghost commented May 13, 2022

Nio set default configurations #27340

Nio set default configurations #27340

Conversation

rickle-msft commented Feb 28, 2022

azure-sdk commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasobol-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickle-msft commented Mar 3, 2022

azure-pipelines bot commented Mar 3, 2022

check-enforcer bot commented Mar 4, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 6, 2022

ghost commented May 13, 2022