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

Springboot: Unable to disable Javers Global Id Cache #878

Closed
duane-staehli opened this issue Sep 6, 2019 · 7 comments
Closed

Springboot: Unable to disable Javers Global Id Cache #878

duane-staehli opened this issue Sep 6, 2019 · 7 comments

Comments

@duane-staehli
Copy link

@duane-staehli duane-staehli commented Sep 6, 2019

When using SpringBoot the JaversSqlAutoConfiguration sets up the "javersSqlRepository" bean. It uses the builder but it does not expose a way to call "withGlobalIdCacheDisabled()".

    @Bean(name = "JaversSqlRepositoryFromStarter")
    @ConditionalOnMissingBean
    public JaversSqlRepository javersSqlRepository(ConnectionProvider connectionProvider) {
        return SqlRepositoryBuilder
                .sqlRepository()
                .withSchema(javersSqlProperties.getSqlSchema())
                .withConnectionProvider(connectionProvider)
                .withDialect(javersSqlDialectName())
                .withSchemaManagementEnabled(javersSqlProperties.isSqlSchemaManagementEnabled())
                .build();
    }

Could you please add the "disable global id cache" property to "JaversSqlProperties" and then honor it in the bean builder.

My work around was to over the bean and add the additional method call in the builder but that isn't very good from a Javer's upgrade path for the applications. Since now I'm tied to ensuring I check that code before upgrade and add any additional changes that might have been made in the builder.

@duane-staehli duane-staehli changed the title Unable to disable Javers Global Id Cache using Springboot Springboot: Unable to disable Javers Global Id Cache Sep 6, 2019
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Sep 6, 2019

Hi, makes sense, consider contributing a pull request

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Sep 7, 2019

but why you want to disable the GlobalIdCache in the first place?

    /**
     * Since 2.7.2, JaversTransactionalDecorator evicts the cache on transaction rollback,
     * so there are no known reasons to disabling it.
     */
    public SqlRepositoryBuilder withGlobalIdCacheDisabled() {
        globalIdCacheDisabled = true;
        return this;
    }

@duane-staehli
Copy link
Author

@duane-staehli duane-staehli commented Sep 10, 2019

Hi Bartosz,

Our experience in multi node systems, is when the cache is out of sync with the current version (i.e the cache is behind). It causes a rollback.

Turning the cache off solves this issue. Without complex caching mechanisms (i.e distributed caching) or forcing the cache to evict itself prior to every save. Given you can't evict individual items, evicting the cache on every node after every update is a lot of work, disabling the cache just avoids this issue. The performance penalty is just not enough to make this worth while.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Sep 11, 2019

ok, understood, consider contributing a PR, we will merge

@akrystian
Copy link
Contributor

@akrystian akrystian commented Oct 24, 2019

I can do PR for this feature.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 28, 2019

The switch will be added, still, I'm not sure about the rationale.

When you have a multi-node system and one node is rolling back a transaction, its local cache is evicted. Since the new primary keys weren't committed, how they can cause troubles on other instances?

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Nov 2, 2019

fixed in 5.8.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants