Skip to content

Allow disabling schema metadata while performing SchemaAction #1255

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow disabling schema metadata while performing SchemaAction #1255

wants to merge 1 commit into from

Conversation

@akhaku
Copy link
Contributor

@akhaku akhaku commented Apr 17, 2022

Additionally, don't force refresh schemas if schema metadata
is disabled.
Closes #990 and #1253

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@akhaku
Disable schema metadata when performing SchemaActions.
f8d9e1a 
Additionally, don't force refresh schemas if schema metadata is disabled.

Closes spring-projects#990 and spring-projects#1253.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 17, 2022
mp911de
mp911de requested changes Apr 21, 2022
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

I reviewed the pull request and left a few comments. In general, making schema refreshes conditional on the configuration makes sense.

...cassandra/src/main/java/org/springframework/data/cassandra/config/CqlSessionFactoryBean.java
executeCql(getStartupScripts().stream(), this.session);
performSchemaAction();
});
} catch (RuntimeException e) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for disabling schema refresh here? And why is there the need to declare and catch exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal here is to disable schema refresh before applying user-provided schema actions. Datastax4 waits for schema refresh after applying changes and if you're applying multiple it's really slow.

Regarding catching and declaring exceptions: it's a little messy because while performSchemaAction here doesn't throw any exceptions, SessionFactoryFactoryBean#performSchemaAction throws Exception and I want to re-use the code to disable and re-enable schema metadata. That requires me to use a ThrowingRunnable (instead of a Runnable) which means that the entire SchemaRefreshUtils.withDisabledSchema method now needs to declare that it throws an Exception. We know that it will never throw a checked exception for CqlSessionFactoryBean but it may throw an unchecked one, so we re-throw any unchecked RuntimeException while wrapping impossible checked exceptions with IllegalStateException just to get the code to compile without propagating throws Exception up the call stack.

The alternative is to use a Runnable instead of ThrowingRunnable in SessionFactoryFactoryBean#performSchemaAction which would mean that #withDisabledSchema doesn't throw any exceptions, but then in SessionFactoryFactoryBean we need to wrap checked exceptions thrown in our performSchemaActions call with RuntimeException to match the Runnable signature.

So yeah a bit of a mess either way. I looked into removing the throws Exception from SessionFactoryFactoryBean#performSchemaAction for a bit but it comes from the AbstractFactoryBean#getObject signature (that's called inside performSchemaAction) so a little difficult.

Copy link
Member

Choose a reason for hiding this comment

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

I see. We can revisit exceptions in performSchemaAction to remove any unnecessary exceptions.

The goal here is to disable schema refresh before applying user-provided schema actions. Datastax4 waits for schema refresh after applying changes and if you're applying multiple it's really slow.

We should leave schema refresh up to the config. If schema refresh is disabled as per config, we don't need to disable it on our side.

Copy link
Contributor Author

@akhaku akhaku Apr 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should leave schema refresh up to the config

for anyone looking at this thread in the future: my impression based on what you said is that we should rely only on schema being disabled via config in performSchemaAction, but I see you added setSuspendLifecycleSchemaRefresh and I'm a big fan!

...cassandra/src/main/java/org/springframework/data/cassandra/config/CqlSessionFactoryBean.java

this.systemSession.refreshSchema();
this.session.refreshSchema();
if (this.systemSession.isSchemaMetadataEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the refresh for the actual session removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually happens inside SchemaRefreshUtils.withDisabledSchema. It isn't obvious from the PR itself, but from the javadoc of session.setSchemaMetadataEnabled:

If calling this method re-enables the metadata (that is, #isSchemaMetadataEnabled() was false before, and becomes true as a result of the call), a refresh is also triggered.

In other words, if schema refresh becomes re-enabled as a result then a refresh will be triggered.

There is one difference though - previously we'd call refreshSchema which is sync and blocking while the schema refresh here is more equivalent to calling Session#refreshSchemaAsync and discarding the returned CompletionStage. I could update the new code here to wait on the returned future but I don't think it's necessary, let me know if you think otherwise!

...cassandra/src/main/java/org/springframework/data/cassandra/config/CqlSessionFactoryBean.java
this.systemSession.refreshSchema();
this.session.refreshSchema();
if (this.systemSession.isSchemaMetadataEnabled()) {
this.systemSession.refreshSchema();
Copy link
Member

Choose a reason for hiding this comment

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

The schema refresh in SessionFactoryFactoryBean should be guarded as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's already tackled in this PR, let me know if I missed something!

mp911de pushed a commit that referenced this pull request Apr 28, 2022
@akhaku @mp911de
Disable schema metadata when performing SchemaActions.
6774457 
Additionally, don't force refresh schemas if schema metadata is disabled.

Closes: #990
Closes: #1253

Original pull request: #1255.
mp911de added a commit that referenced this pull request Apr 28, 2022
@mp911de
Polishing.
a654a45 
Introduce configuration option to disable schema metadata during schema actions. Tweak method naming. Apply schema metadata suspension also to destroy methods.

Related ticket: #990
Related ticket: #1253

Original pull request: #1255.
mp911de pushed a commit that referenced this pull request Apr 28, 2022
@akhaku @mp911de
Disable schema metadata when performing SchemaActions.
1d1505a 
Additionally, don't force refresh schemas if schema metadata is disabled.

Closes: #990
Closes: #1253

Original pull request: #1255.
mp911de added a commit that referenced this pull request Apr 28, 2022
@mp911de
Polishing.
6d14464 
Introduce configuration option to disable schema metadata during schema actions. Tweak method naming. Apply schema metadata suspension also to destroy methods.

Related ticket: #990
Related ticket: #1253

Original pull request: #1255.
@mp911de mp911de changed the title Disable schema metadata when performing SchemaActions Allow disabling schema metadata while performing SchemaAction Apr 28, 2022
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 28, 2022
@mp911de mp911de added this to the 3.4 GA (2021.2.0) milestone Apr 28, 2022
@mp911de
Copy link
Member

mp911de commented Apr 28, 2022

Thank you for your contribution. That's merged, polished, and backported now.

@mp911de mp911de closed this Apr 28, 2022
@akhaku akhaku deleted the cassandraRefresh branch April 30, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mp911de mp911de mp911de requested changes

Assignees

No one assigned

Labels

type: enhancement A general enhancement

Projects

None yet

Milestone

3.4 GA (2021.2.0)

Development

Successfully merging this pull request may close these issues.

Allow disabling schema metadata while performing SchemaAction [DATACASS-823]

3 participants

@akhaku @mp911de @spring-projects-issues