Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KEYCLOAK-10954] - Cache invalidation Javascript policies - cluster mode #6206

Closed
wants to merge 1 commit into from

Conversation

pedroigor
Copy link
Contributor

No description provided.

Copy link
Contributor

@abstractj abstractj left a comment

Choose a reason for hiding this comment

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

@pedroigor I just commented on the same Jira. But it seems the changes you've been working for JS engine will supersede this PR.

@pedroigor
Copy link
Contributor Author

@abstractj we need this one.

@abstractj abstractj requested a review from stianst August 26, 2019 17:25
@stianst
Copy link
Contributor

stianst commented Sep 2, 2019

@pedroigor Can you explain why caching of JS policies are needed? It seems now there's no need for invalidation cache here, but rather it is just loaded from the filesystem and re-loaded on re-deploy?

@pedroigor
Copy link
Contributor Author

pedroigor commented Sep 6, 2019

@stianst this cache avoids unnecessary compilation of scripts every time they are executed. Thus helping to boost execution during runtime (I did this some time ago when looking authz performance). Even with the new approach for deploying scripts into the server, we still need this cache layer because of that.

I understand that with the new way of deploying scripts to the server, we don't need it anymore. And we should remove this once we completely remove script upload from the server.

@pedroigor
Copy link
Contributor Author

@stianst can we have this one approved?

Copy link
Contributor

@slaskawi slaskawi left a comment

Choose a reason for hiding this comment

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

Approved.

Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

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

As far as I can see this PR is still focusing on the old approach of uploading JS over REST, and is not the correct approach for the new approach. It is also only focusing on JS policies and not all JS code.

@stianst
Copy link
Contributor

stianst commented Oct 17, 2019

@stianst this cache avoids unnecessary compilation of scripts every time they are executed. Thus helping to boost execution during runtime (I did this some time ago when looking authz performance). Even with the new approach for deploying scripts into the server, we still need this cache layer because of that.

I understand that with the new way of deploying scripts to the server, we don't need it anymore. And we should remove this once we completely remove script upload from the server.

That seems to be two contradicting statements. #1 "Even with the new approach for deploying scripts into the server, we still need this" and #2 "with the new way of deploying scripts to the server, we don't need it anymore".

From my understanding of this PR it is not relevant with the new approach, and we should not make improvements/changes to what is a deprecated and no longer recommended approach.

When deploying JS code now I assume it is wrapped in a factory, where the factory instance should hold the compiled version of the JS. This renders a invalidation cache like this useless.

@pedroigor
Copy link
Contributor Author

Yes, for the new strategy of deploying scripts to the server it is not needed anymore. But when using the deprecated way of doing so.

Closing.

@pedroigor pedroigor closed this Oct 17, 2019
@pedroigor pedroigor deleted the KEYCLOAK-10954 branch August 3, 2021 12:10
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.

None yet

4 participants