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

event store #124

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

event store #124

wants to merge 17 commits into from

Conversation

xgp
Copy link

@xgp xgp commented Jan 9, 2024

Adding an EventStoreProvider implementation per #3

Submitting a DRAFT PR for feedback.

Current issues:

Schema questions

Need feedback on the schema, as I'm not experienced with Cassandra https://github.com/xgp/keycloak-cassandra-extension/blob/xgp/event-store/core/src/main/resources/cassandra/migration/0003_events.cql

Admin event resourcePath LIKE

Because LIKE queries require secondary indexes to be enabled in Cassandra, they are not currently supported for querying resourcePath of admin events.

Query performance

Performance of event queries using sparse fields (i.e. those generated by the Admin UI) are not performant, as Cassandra is forced to use ALLOW FILTERING. Future work could include a separate lookup table with client-side result filtering.

Pagination performance

Because Cassandra does not support OFFSET/LIMIT type queries, this is done in Java. This will likely consume significant resources for large event result sets.

@xgp xgp mentioned this pull request Jan 9, 2024
Copy link
Owner

@opdt opdt left a comment

Choose a reason for hiding this comment

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

Please squash your commits to one.

@@ -164,9 +172,20 @@ public void init(Config.Scope scope) {
repository = createRepository(cqlSession);
}

public CqlSessionBuilder configure(CqlSessionBuilder cqlSessionBuilder) {
Copy link
Owner

Choose a reason for hiding this comment

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

This should be done with external file based configuration and not hardcoded

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @opdt I was having problems getting the migration to run in the tests without a timeout. Can you point me to some documentation/examples of how this is achieved using external file based configuration? I'm not super familiar with cassandra, so the options are new to me.

Copy link
Author

Choose a reason for hiding this comment

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

I've been struggling to figure out how to load an external file based configuration @opdt. Is there an example you're aware of?

Copy link
Author

Choose a reason for hiding this comment

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

Finally figured out how to load config from a file. I have a branch here, so let me know if you want me to do a separate PR. https://github.com/opdt/keycloak-cassandra-extension/compare/main...xgp:keycloak-cassandra-extension:xgp/config-file?expand=1

Copy link
Owner

Choose a reason for hiding this comment

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

You dont need any custom code to load the config from a file. The cassandra driver does it automatically: https://docs.datastax.com/en/developer/java-driver/4.17/manual/core/configuration/ (application.conf in Classpath)

Copy link
Author

Choose a reason for hiding this comment

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

Correct. But if you want to allow it to be put somewhere else and not bundled with the jar, then you have to tell Cassandra where to load it from.

Copy link
Author

Choose a reason for hiding this comment

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

For example, I might want to have k8s mount the conf file in the pod and then need to tell the extension where to load it from. This decouples the configuration from the library.

Copy link
Owner

@opdt opdt Feb 20, 2024

Choose a reason for hiding this comment

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

Thats possible via -Dconfig.file=/path/to/application.confvia the underyling Lightbend configuration library (which also supports loading from other resources/URLs).
You can pass additional Java arguments via JAVA_OPTS_APPEND environment variable in Keycloak.

Copy link
Author

Choose a reason for hiding this comment

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

The problem I encounter when I specify that is that the init method still runs. So even though the underlying library supports the -Dconfig.file override, it appears that it never gets there because of the exceptions thrown when the variables are missing.

Also, if I set both, the variables in the config scope are what gets used.

docker-compose.yaml Outdated Show resolved Hide resolved
tests/pom.xml Show resolved Hide resolved
tests/pom.xml Outdated Show resolved Hide resolved
@xgp
Copy link
Author

xgp commented Jan 13, 2024

Thank you @opdt Most requested updates made. I'll squash once we're done with all the changes. Please see my comments on two of the issues and let me know your answers/recommendations.

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

2 participants