Keyvault performance by mnriem · Pull Request #905 · microsoft/azure-spring-boot

mnriem · 2020-05-29T14:32:16Z

Summary

Rework refreshing logic to increase performance

Issue Type

Bug fixing

Starter Names

key vault spring boot starter

Additional Information

azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultOperation.java

...spring-boot/src/test/java/com/microsoft/azure/keyvault/spring/CaseSensitiveKeyVaultTest.java

chenrujun · 2020-06-02T06:11:37Z

azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultOperation.java

+            };
+            timer.scheduleAtFixedRate(task, 0, refreshInMillis);
+        } else {
+            refreshProperties();


If refreshInMillis <= 0, we will not use cache, we should get the result(secret name list and secret value) from key-vault in every request.

Also not going to change this as this setting is for refreshing the cached values, not for not caching at all. Should we allow for no caching at all? I do not think that is wise as the underlying Spring runtime can also be calling this at any given time (we do not control that).

Should we allow for no caching at all?

I already discussed with @jialindai , we think that we allow no caching at all.

Let user configure whether need to cache.

If user configure refreshInMillis == 0, not cache make sense.

Maybe can can use kay-vault.cache.ms instead of refresh-interval as configure item?

No caching will be detrimental to any application as you are not in control of when the PropertySource gets consulted. I strongly advice against that.

Your concern make sense.

But current behavior is no cache.

IMU,

About 80%(just guess) of key-vault property will only get one time. (when start the app).

It's OK to let our customer determine whether to use cache.

If we set the refresh interval to -1 to indicate we do NO caching at all then I will go ahead and implement that, but I still think this is bad from a user experience point of view. Our code now can slow down any part of their application because we do NOT control the order in which PropertySources are consulted, nor at what time! So agreed on the -1?

As discussed in email. Let our customer to decide the TTL of cache.

azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultOperation.java

chenrujun · 2020-06-02T06:30:18Z

azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultOperation.java

+                    refreshProperties();
+                }
+            };
+            timer.scheduleAtFixedRate(task, 0, refreshInMillis);


If refreshInMillis equal to 1 second, and task will cost 1 minute to finish, then many task will run at the same time? That's not good.

And it's not necessary to refreshProperties() again and again if the app only need to read the properties once.

(cc @jialindai FYI.)

I mean, if getPropertyNames() and getProperty are not called, refreshProperties() should not execute.

I disagree. A refresh interval is exactly that, aka the interval after which the keys/values are refreshed from the backend. So not going to change this. If the application developer does not need a refresh of the keys/values after startup the refresh interval can be set to 0.

From the JavaDoc of Timer, "Corresponding to each Timer object is a single background thread that is used to execute all of the timer's tasks, sequentially."

I feel you are looking to implement a TTL for each secret, if so we would have to rearchitect this.

Not for each secret, use current configure item refresh-interval as TTL of all secrets is my purpose.

Can we proceed as it currently stands? And IF we decide that TTL is appropriate to do that later?

Can we use current configure item refresh-interval as TTL of all secrets?

But even with that said why on earth would anyone want to refresh the cache every minute?

Just as I said above: our customer may update key-value pair in key-vault (manually or by java code), then get the pair by environment.getProperty() minutes latter. if customer can not get the updated data, it may make our customer confused.

I feel you are looking to implement a TTL for each secret, if so we would have to rearchitect this.

Not for each secret, use current configure item refresh-interval as TTL of all secrets is my purpose.

Can we proceed as it currently stands? And IF we decide that TTL is appropriate to do that later?

Can we use current configure item refresh-interval as TTL of all secrets?

Great then I can leave the code I wrote as is as that is what the refresh interval does. It refreshes all the secrets after a specific time (Time To Live)

But even with that said why on earth would anyone want to refresh the cache every minute?

Just as I said above: our customer may update key-value pair in key-vault (manually or by java code), then get the pair by environment.getProperty() minutes latter. if customer can not get the updated data, it may make our customer confused.

See my comments above

It refreshes all the secrets after a specific time (Time To Live)

To save the resource of network and key-vault server:

I mean, if getPropertyNames() and getProperty are not called, refreshProperties() should not execute.

chenrujun · 2020-06-09T05:52:41Z

Hi, @mnriem ,
(cc @yiliuTo , FYI).

Is there any progress in this PR?
If you are busy, maybe I can help to finish the logic of cache key vault values.

chenrujun · 2020-06-16T01:48:06Z

@jialindai said that

Manfred had some discussion with us on PR 905, the conclusion is go ahead with Manfred’s current approach which eagerly pull key vault. Please help to approve it.

There may be some further fine tuning, which will be tracked by further PR.

So I'll approve it now.

AbhijeetAvaghade · 2020-07-28T07:11:51Z

if this is part of 2.3.2 release?

yiliuTo · 2020-07-28T07:28:40Z

if this is part of 2.3.2 release?

We plan to release it in 2.3.3 version

AbhijeetAvaghade · 2020-07-28T09:50:49Z

Any tentative date for 2.3.3 release?

yiliuTo · 2020-07-29T06:32:55Z

Any tentative date for 2.3.3 release?

We plan to release 2.3.3 around mid-August.

mnriem added 7 commits May 29, 2020 07:21

Remove nbactions.xml

579485e

Rework refreshing logic

16a263e

Iterate over secrets by page

a8664b3

Iterate over secrets by page

fe95072

Iterate over secrets by page

39ab650

Iterate over secrets by page

32564b8

Make sure unit tests pass

af66395

mnriem mentioned this pull request Jun 1, 2020

Keyvault Spring Boot Actuator support #896

Merged

chenrujun suggested changes Jun 2, 2020

View reviewed changes

chenrujun mentioned this pull request Jun 3, 2020

[Feature Request] Key Vault Secrets only load on startup #907

Closed

yiliuTo linked an issue Jun 9, 2020 that may be closed by this pull request

[Bug] Key Vault Secrets loaded multiple times #906

Closed

Rujun Chen and others added 3 commits June 10, 2020 09:33

Merge branch 'master' into keyvault-performance

5a17ed3

Reorder refreshProperties

2bdf8fd

Apply requested changes. Made duplicateKey test use case sensitive keys

bd14e1a

mnriem marked this pull request as ready for review June 10, 2020 19:53

chenrujun mentioned this pull request Jun 12, 2020

Key vault property source support actuator and cache key vault vaules #920

Closed

Merge branch 'master' into keyvault-performance

9e6e891

chenrujun approved these changes Jun 16, 2020

View reviewed changes

chenrujun merged commit 18fb06d into microsoft:master Jun 16, 2020

saragluna mentioned this pull request Jun 16, 2020

Apply all PRs created after migration began to azure-sdk-for-java repo #916

Closed

21 tasks

Conversation

mnriem commented May 29, 2020

Summary

Issue Type

Starter Names

Additional Information

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenrujun commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenrujun commented Jun 16, 2020

Uh oh!

AbhijeetAvaghade commented Jul 28, 2020

Uh oh!

yiliuTo commented Jul 28, 2020

Uh oh!

AbhijeetAvaghade commented Jul 28, 2020

Uh oh!

yiliuTo commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chenrujun commented Jun 9, 2020 •

edited

Loading

yiliuTo commented Jul 29, 2020 •

edited

Loading