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

Clusterable JaVers #411

Closed
bartoszwalacik opened this issue Jul 31, 2016 · 11 comments
Closed

Clusterable JaVers #411

bartoszwalacik opened this issue Jul 31, 2016 · 11 comments
Assignees
Labels

Comments

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Jul 31, 2016

JaVers should not fail when an applictions which uses JaVers is deployed in clustered environment.
Which means many JVMs with JaVers instances are writing to single database instance (single JaVersRepository).

At least two enhancements should be implemented:

  • CommitId generator have to be reimplemented using database synchronization to provide subsequent CommitId numbers. In SQL it could be a sequence.
  • Snapshot.version field is vulnerable for concurrent writes. We can try with optimistic locking algorithm here (exception will be thrown on concurrent updates of the same domain object)

Good integration test should be created (many JaVers instances writing to the same DB)

@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Jul 31, 2016

@edmallia at least we have an issue

Loading

@achimbitzer
Copy link

@achimbitzer achimbitzer commented Oct 11, 2016

We are using JaVers in our application and also discovered that JaVers is not clusterable at the moment. Our application runs in a redundant / clustered environment with two application instances, therefore we vote for this feature!

We were running jmeter tests on our application in a redundant setup. The following error popped up multiple times:

org.javers.common.exception.JaversException: CANT_SAVE_ALREADY_PERSISTED_COMMIT can't save already persisted commit '3089.1' at org.javers.repository.sql.JaversSqlRepository.persist(JaversSqlRepository.java:59) ~[JaversSqlRepository.class:?] at org.javers.repository.api.JaversExtendedRepository.persist(JaversExtendedRepository.java:136) ~[JaversExtendedRepository.class:?] at org.javers.core.JaversCore.commit(JaversCore.java:79) ~[JaversCore.class:?] at org.javers.spring.jpa.JaversTransactionalDecorator.commit(JaversTransactionalDecorator.java:59) ~[JaversTransactionalDecorator.class:?]

After looking into the source this seems to be the CommitId generator issue mentioned in the first comment on this issue. The same CommitId has been generated by multiple application instances. As far as I can see in the source code this kind of error is possible because the CommitId generator uses an in-memory storage of already handed out CommitIds to generate new unique ids.

Currently we are looking for a workaround to be able to use JaVers in our clustered environment that does not produce this error message. Is there a known workaround?

EDIT: some more questions :)

For example it would be great if it would be possible to use a custom implementation of CommitSeqGenerator, but as far as I can see this is statically linked in JaversBuilder via CommitFactoryModule.

Is there any mechanism to intercept Javers initialization and modify the core beans accordingly or to add a custom module to the pico container for generating unique CommitIds?

Loading

@bgalek
Copy link
Contributor

@bgalek bgalek commented Oct 11, 2016

maybe CommitGenerator could have a seed parameter (i.e. ip, hostname, deployment id) for more uniqe ids?

Loading

@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Oct 11, 2016

There is no workaround for this issue. As you suggest, JaVers should allow passing custom implementation of CommitSeqGenerator. The simplest impl for a clustered env can use UUID.
I'll try to put some work on this feature

Loading

@bgalek
Copy link
Contributor

@bgalek bgalek commented Oct 12, 2016

I could do it next week ;)

Loading

@achimbitzer
Copy link

@achimbitzer achimbitzer commented Oct 12, 2016

Sounds great, thanks for your quick replies!

Loading

@philnate
Copy link

@philnate philnate commented Oct 19, 2016

Sorry for asking, but isn't this problem as well existing with multi-threading, without going into clustering?
We had to explicitly synchronize around the javers executions to avoid that our commitIds had big increases in the minor value of the id. This is caused by the fact that the commitId update is processed 3-folded.

  • First gathering the last used id
  • Incrementing the id
  • Persisting the latest id

As this isn't happening atomically it's easily possible to have a high churn on minor, while I guess this should rather be the exception. I've observed this with javers 1.2, not sure if this is still the case with 2.x

Loading

@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Oct 20, 2016

As far as I know CommitSeqGenerator is now thread-safe. v 1.2 is really old, could you try latest version (2.4.1) and let me know if it helps?

Loading

@Waahh
Copy link

@Waahh Waahh commented Oct 26, 2016

Hi, are there any news regarding this issue?

Loading

@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Oct 27, 2016

keep calm, I'm going to implement some simple alg with random UIDs and release it as EAP for testing

Loading

bartoszwalacik added a commit that referenced this issue Oct 27, 2016
DistributedCommitSeqGenerator
bartoszwalacik added a commit that referenced this issue Oct 28, 2016
@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Oct 30, 2016

fixed in release 2.6.0
see http://javers.org/release-notes/

Loading

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

Successfully merging a pull request may close this issue.

None yet
5 participants