- 
                Notifications
    You must be signed in to change notification settings 
- Fork 310
          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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -67,6 +67,7 @@ | |
| * @author John Blum | ||
| * @author Mark Paluch | ||
| * @author Tomasz Lelek | ||
| * @author Ammar Khaku | ||
| * @since 3.0 | ||
| */ | ||
| public class CqlSessionFactoryBean | ||
|  | @@ -450,11 +451,20 @@ public void afterPropertiesSet() { | |
|  | ||
| this.session = buildSession(sessionBuilder); | ||
|  | ||
| executeCql(getStartupScripts().stream(), this.session); | ||
| performSchemaAction(); | ||
| try { | ||
| SchemaRefreshUtils.withDisabledSchema(this.session, () -> { | ||
| executeCql(getStartupScripts().stream(), this.session); | ||
| performSchemaAction(); | ||
| }); | ||
| } catch (RuntimeException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Unexpected checked exception thrown", e); | ||
| } | ||
|  | ||
| this.systemSession.refreshSchema(); | ||
| this.session.refreshSchema(); | ||
| if (this.systemSession.isSchemaMetadataEnabled()) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the refresh for the actual session removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It actually happens inside  
 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  | ||
| this.systemSession.refreshSchema(); | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The schema refresh in  There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! | ||
| } | ||
| } | ||
|  | ||
| protected CqlSessionBuilder buildBuilder() { | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /* | ||
| * Copyright 2022 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|  | ||
| package org.springframework.data.cassandra.config; | ||
|  | ||
| import com.datastax.oss.driver.api.core.session.Session; | ||
|  | ||
| /** | ||
| * Utility methods for executing schema actions with refresh disabled. | ||
| * | ||
| * @author Ammar Khaku | ||
| */ | ||
| class SchemaRefreshUtils { | ||
| @FunctionalInterface | ||
| interface ThrowingRunnable { | ||
| void run() throws Exception; | ||
| } | ||
|  | ||
| /** | ||
| * Programmatically disables schema refreshes on the session and runs the provided Runnable, | ||
| * taking care to restore the previous state of schema refresh config on the provided session. | ||
| * Note that the session could have had schema refreshes enabled/disabled either | ||
| * programmatically or via config. | ||
| */ | ||
| static void withDisabledSchema(Session session, ThrowingRunnable r) throws Exception { | ||
| boolean schemaEnabledPreviously = session.isSchemaMetadataEnabled(); | ||
| session.setSchemaMetadataEnabled(false); | ||
| r.run(); | ||
| session.setSchemaMetadataEnabled(null); // triggers schema refresh if results in true | ||
| if (schemaEnabledPreviously != session.isSchemaMetadataEnabled()) { | ||
| // user may have set it programmatically so set it back programmatically | ||
| session.setSchemaMetadataEnabled(schemaEnabledPreviously); | ||
| } | ||
| } | ||
| } | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /* | ||
| * Copyright 2022 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|  | ||
| package org.springframework.data.cassandra.config; | ||
|  | ||
| import static org.mockito.Mockito.*; | ||
|  | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.mockito.Mock; | ||
| import org.mockito.junit.jupiter.MockitoExtension; | ||
|  | ||
| import com.datastax.oss.driver.api.core.session.Session; | ||
|  | ||
| /** | ||
| * Test suite of unit tests testing the contract and functionality of the {@link SchemaRefreshUtils} class. | ||
| */ | ||
| @ExtendWith(MockitoExtension.class) | ||
| class SchemaRefreshUtilsUnitTests { | ||
| @Mock Session session; | ||
|  | ||
| @Test | ||
| void withDisabledSchemaRevert() throws Exception { | ||
| when(session.isSchemaMetadataEnabled()).thenReturn(true); | ||
| SchemaRefreshUtils.withDisabledSchema(session, () -> {}); | ||
| verify(session).setSchemaMetadataEnabled(false); | ||
| verify(session).setSchemaMetadataEnabled(null); | ||
| } | ||
|  | ||
| @Test | ||
| void withDisabledSchemaDisabledPreviously() throws Exception { | ||
| when(session.isSchemaMetadataEnabled()).thenReturn(false); | ||
| SchemaRefreshUtils.withDisabledSchema(session, () -> {}); | ||
| verify(session).setSchemaMetadataEnabled(false); | ||
| verify(session).setSchemaMetadataEnabled(null); | ||
| } | ||
|  | ||
| @Test | ||
| void withDisabledSchemaDisabledProgrammaticallyPreviously() throws Exception { | ||
| when(session.isSchemaMetadataEnabled()).thenReturn(false).thenReturn(true); | ||
| SchemaRefreshUtils.withDisabledSchema(session, () -> {}); | ||
| verify(session, times(2)).setSchemaMetadataEnabled(false); | ||
| verify(session).setSchemaMetadataEnabled(null); | ||
| } | ||
|  | ||
| @Test | ||
| void withDisabledSchemaEnabledProgrammaticallyPreviously() throws Exception { | ||
| when(session.isSchemaMetadataEnabled()).thenReturn(true).thenReturn(false); | ||
| SchemaRefreshUtils.withDisabledSchema(session, () -> {}); | ||
| verify(session).setSchemaMetadataEnabled(true); | ||
| verify(session).setSchemaMetadataEnabled(null); | ||
| } | ||
| } | 
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
performSchemaActionhere doesn't throw any exceptions,SessionFactoryFactoryBean#performSchemaActionthrowsExceptionand I want to re-use the code to disable and re-enable schema metadata. That requires me to use aThrowingRunnable(instead of aRunnable) which means that the entireSchemaRefreshUtils.withDisabledSchemamethod now needs to declare that it throws anException. We know that it will never throw a checked exception forCqlSessionFactoryBeanbut it may throw an unchecked one, so we re-throw any uncheckedRuntimeExceptionwhile wrapping impossible checked exceptions withIllegalStateExceptionjust to get the code to compile without propagatingthrows Exceptionup the call stack.The alternative is to use a
Runnableinstead ofThrowingRunnableinSessionFactoryFactoryBean#performSchemaActionwhich would mean that#withDisabledSchemadoesn't throw any exceptions, but then inSessionFactoryFactoryBeanwe need to wrap checked exceptions thrown in ourperformSchemaActionscall withRuntimeExceptionto match theRunnablesignature.So yeah a bit of a mess either way. I looked into removing the
throws ExceptionfromSessionFactoryFactoryBean#performSchemaActionfor a bit but it comes from theAbstractFactoryBean#getObjectsignature (that's called insideperformSchemaAction) so a little difficult.There was a problem hiding this comment.
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
performSchemaActionto remove any unnecessary exceptions.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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 addedsetSuspendLifecycleSchemaRefreshand I'm a big fan!