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

Javers fails under load in distributed applications #657

Closed
gbondarchuk9 opened this issue Apr 5, 2018 · 33 comments
Closed

Javers fails under load in distributed applications #657

gbondarchuk9 opened this issue Apr 5, 2018 · 33 comments
Labels

Comments

@gbondarchuk9
Copy link

Javers fails under load in distributed applications during commit: org.javers.common.exception.JaversException: CANT_SAVE_ALREADY_PERSISTED_COMMIT: can't save already persisted commit '37140.0'.

We use MySQL database, and Spring integration module.

Seems like the following situation occurs:

  1. On the first application instance we called javers.commit(...) in transaction - it retrieved max commit_id from database and then increased it.
  2. On the second application instance we also called javers.commit(...) in another transaction - it retrieved the same max commit_id as in step Initial diff #1 and also increased it by one. (first transaction is not committed yet)
  3. First application instance committed transaction - data successfully saved to database.
  4. Second application instance committed transaction - we got exception as above, as data with the same commit_id already exist in database after first transaction completion.

_We saw that CommitIdGenerator contains both: SYNCHRONIZED_SEQUENCE and RANDOM (for distributed applications), but RANDOM generator is deprecated, do we have some alternative generator for distributed applications? What can we undertake to fix the issue?

Full Stacktrace
stacktrace.txt

@gbondarchuk9 gbondarchuk9 changed the title Javers Fails Under Load in distributed applications Javers fails under load in distributed applications Apr 5, 2018
@bartoszwalacik
Copy link
Member

thanks for reporting. Indeed, there are problems with id generator for distributed apps. We need to invent sth better than RANDOM. Any suggestions?

@gbondarchuk9
Copy link
Author

gbondarchuk9 commented Apr 6, 2018

Hi Bartosz!

Thanks for the reply.

We have a couple suggestions/improvements, so:

  1. We propose to make CommitIdGenerator pluggable and available for passing appropriate implementation thru JaversConfiguration, so we can provide our own generator. It requires to make a general interface for generators. (Exactly the same approach as for ObjectAccessHook).
    So we would like to have the following structure:
public interface CommitIdGenerator {
   public CommitId nextId();
}

public class CustomCommitIdGenerator implements CommitIdGenerator {
   public CommitId nextId() {
      return ...;
   }
}

JaversBuilder builder = TransactionalJaversBuilder.javers()
...
.withCommitIdGenerator(new CustomCommitIdGenerator())
...
  1. Create CommitId based on UUID or md5 hash of current timestamp + some unique identifier for application instance (it can be current thread number, process id, etc), and changed commid_id data type from decimal (22,2) to varchar(30), for example.

--
thanks

@bartoszwalacik
Copy link
Member

That's how CommitIdGenerator.RANDOM works. You can use it but it would break Shadow queries

    /**
     * @deprecated RANDOM CommitIdGenerator is deprecated because it don't play along with Shadow queries. Only the default algorithm (SYNCHRONIZED_SEQUENCE) is supported.
     * @since 2.6
     */
    @Deprecated
    public JaversBuilder withCommitIdGenerator(CommitIdGenerator commitIdGenerator) {
    

The main problem is -- ShadowQueryRunner sorts by commitId, so CommitIdGenerator must produce the monotonically increasing sequence

@bartoszwalacik
Copy link
Member

Maybe we should use a DB sequence instead?

@gbondarchuk9
Copy link
Author

The DB sequence might be a bottleneck for highly loaded applications (but this definitely very helpful for medium size systems), also this is not a good choice for MongoDB. Anyway we can make CommitIdGenerator pluggable and create Pull Request.

Can we make ShadowQueryRunner to sort by, for example, creation timestamp instead of commit_id? Is the commit_id used for anything else except sorting?

@bartoszwalacik
Copy link
Member

Pluggable CommitIdGenerator will add more complexity. What we need is good CommitIdGenerator implementation for distributed apps, and it should be available in JaVers out of the box.

CommitId is used for sorting (by time) in Shadow queries (see JaversExtendedRepository.getHistoricals()).
It should be monotonically increasing. Creation timestamp is not very good for that because you can have diverged clocks among servers.

I think about some mixed apporoach, majorId could be generated as in SYNCHRONIZED_SEQUENCE, so head +1 and minor Id as random, for example

1.7489099
2.3424455
3.4425523

This would give us almost perfect uniqueness and good time-based orderability. Should be good enough for Shadow queries. What do you think?

@sergey-derugo
Copy link

sergey-derugo commented Apr 10, 2018

Bartosz
If you have diverged clocks among servers then most likely you have plenty of other problems so usually distributed application significantly relies on time synchronization.
Also using DB sequence introduces issues for highly loaded systems, in particular:

  1. DB sequence might become a bottleneck for the hole system because all transactions constantly need query next value.
  2. Some databases doesn’t support sequence, for example, MySQL has AUTO_INCREMENT option while Oracle support sequences.
  3. Implementation of sequence might be tricky for the MongoDB because usually people prefer avoid synchronization mechanism and requirement about monotonic numbers makes the things much more complex.
    My suggestion is that we should define Commit Id as following:
	Major = “Entity Class @ Entity Id” or hash(“Entity Class” + ”Entity Id”)
	Minor = “select now()” or “current timestamp” (if NoSQL database is used)

For example, if there is entity com.app.Foo {id=123, name=”Bla-bla-bal”} then every commit might have the following ID:

	Major = com.app.Foo @ 123
	Minor = 23456789 

This way we can have unique numbers for commits and provide good support of distributed mode.

Thanks

@bartoszwalacik
Copy link
Member

bartoszwalacik commented Apr 10, 2018

I'm not insisting on DB sequence, in fact SYNCHRONIZED_SEQUENCE is not using DB sequence, the implementation is DB independant. I like the idea to use commitDate (timestamp) for sorting. We already have it so we can us it. So my suggestion:

  • un-deprecate CommitIdGenerator.RANDOM
  • ShadowQueryRunner would use commitDate for sorting (instead of CommitId)
  • maybe db index should be added on commitDate field

@gbondarchuk9
Copy link
Author

Seems like it will work.

Unfortunately, I don't have enough knowledge about ShadowQueryRunner and Javers internals, so I'm afraid I cannot create right Pull Request.

Could you please make these changes in the next weeks?

@bartoszwalacik
Copy link
Member

Will try at weekend

@gbondarchuk9
Copy link
Author

Thanks, Bartosz!

@bartoszwalacik
Copy link
Member

the PR is ready https://github.com/javers/javers/pull/661/files

@bartoszwalacik
Copy link
Member

ok, the fix is released in 3.9.1
It will be available in a few hours in Central.
RANDOM generator is un-deprecated and there are changes in ShadowQueryRunner around commits sorting.

@bartoszwalacik
Copy link
Member

Try it, and let me know if it works for you.

@gbondarchuk9
Copy link
Author

Hi Bartosz!

Thanks for the changes you've made. I found a couple issues regarding that:

  1. Using Random generator shadow query returns one shadow as using Synchronized Sequence generator it returns two shadows (both commits have equal commit_date field).
  2. After migrating from Random to Synchronized Sequence generator, none of commits can be persisted:
JaversException CANT_SAVE_ALREADY_PERSISTED_COMMIT: can't save already persisted commit '6624663025432992769.0'
JaversException CANT_SAVE_ALREADY_PERSISTED_COMMIT: can't save already persisted commit '6624663025432992769.1'
JaversException CANT_SAVE_ALREADY_PERSISTED_COMMIT: can't save already persisted commit '6624663025432992769.2'

@bartoszwalacik
Copy link
Member

bartoszwalacik commented Apr 19, 2018

Hi,
RANDOM generator always sets 0 as minorId (see https://github.com/javers/javers/blob/master/javers-core/src/main/java/org/javers/core/commit/DistributedCommitSeqGenerator.java).

So this commitId - 6624663025432992769.2 is probably generated by SEQUENCE generator.
Looks like, you have different javers' configurations on one app cluster.

@bartoszwalacik
Copy link
Member

Concerning the first issue, please provide a runnable test. I need to reproduce it on my side.

@gbondarchuk9
Copy link
Author

gbondarchuk9 commented Apr 19, 2018

I tested that on single app instance, not a cluster.

6624663025432992769.0 was generated by RANDOM generator, when I switched to SEQUENCE generator which increased the last commit id to 6624663025432992769.1

@bartoszwalacik
Copy link
Member

I don't understand why you are switching to SEQUENCE. The whole issue is about using RANDOM generator.

@bartoszwalacik
Copy link
Member

bartoszwalacik commented Apr 20, 2018

Released in 3.9.1

Javers javers = javers().withCommitIdGenerator(CommitIdGenerator.RANDOM)
                        .build();

@gbondarchuk9
Copy link
Author

Hi Bartosz,

  1. Regarding to switching to SEQUENCE, I meant that if somebody used RANDOM and then decided to switch to SEQUENCE - it would never work. (see attached test
    CommitIdGeneratorTest.txt
    )

  2. Concerning the issue when shadow query returns one shadow instead of two, unfortunately I couldn't write test which shows the issue (because it works for in-memory repository). But if I have two records in jv_commit table with commit_date = '2018-04-22 14:15:07' it returns one shadow, once I change one commit_date directly to '2018-04-22 14:15:08' it starts to return two shadows - thereby the issue definitely exists.

--
thanks,
Gleb

@bartoszwalacik
Copy link
Member

  1. You can change from SEQUENCE to RANDOM but you can't change from RANDOM to SEQUENCE.
    When SEQUENCE generator is used, commits are sorted by CommitId (which obviously doesn't make any sense for commits generated by RANDOM).
    When RANDOM generator is used, commits are sorted by commitDate.

  2. Could you come up with a failing test case? You can push it to your fork of Javers.

@gbondarchuk9
Copy link
Author

I've added a test - gbondarchuk9@75467e0, but it works. Will try to find a way to get it failed.

What I do is: execute two commits inside one transaction (just one by one):

  • start transaction
  • javers.commit(author, entity, properties)
  • entity = change(entity) //change some entity fields
  • javers.commit(author, entity, properties)
  • commit transaction - it produces two records in jv_shapshot table and two records in jv_commit table (commit date values are identical)
  • then query for shadows: it returns only one shadow related to the second commit.

Either I modify one of commit dates by second (+/-), or do the same step as above under SEQUENCE generator - it works fine.

@bartoszwalacik
Copy link
Member

So you are updating an object twice in the same millisecond? How Javers would know which snapshot is the last one? Remember that commits are sorted by commitDate when RANDOM is used. The whole algorithm will work as soon as you don't update on object from two different threads (or two javers instances) exactly in the same millisecond.

@gbondarchuk9
Copy link
Author

gbondarchuk9 commented Apr 23, 2018

Yes, I have such functionality that requires two object states (which is saved in the same second, not millisecond). Sure, Javers won't know which snapshot will be last and which will be first, but I supposed it had to return both snapshots with the same commitDate instead of one, as commitDate is used only for sorting, didn't I? I'm using single instance app, not a cluster.

Is it possible to widen commitDate to use milliseconds?

--
thanks,
Gleb

@bartoszwalacik
Copy link
Member

commitDate already has milliseconds precision

@bartoszwalacik
Copy link
Member

Please let me know when you come up with a failing test with the reproduction of the issue you are writing about

@gbondarchuk9
Copy link
Author

Hi Bartosz,

As far as I know in MySQL to have fractional precision (milliseconds, microseconds), column type should be TIMESTAMP(1) and higher (TIMESTAMP(3) in our case, I think), but currently we have pure TIMESTAMP type for commitDate.
https://dev.mysql.com/doc/refman/8.0/en/datetime.html
please, correct me if I'm worng

--
thanks,
Gleb

@bartoszwalacik
Copy link
Member

could be, I've checked only Mongo and Postres. Feel free to contribute a PR with the fix here https://github.com/polyjdbc/polyjdbc/blob/master/src/main/java/org/polyjdbc/core/dialect/MysqlDialectTypes.java

@gbondarchuk9
Copy link
Author

gbondarchuk9 commented Apr 24, 2018

I've created PR
polyjdbc/polyjdbc#38

@bartoszwalacik
Copy link
Member

MySql issue is moved here #664

@a-mujthaba321
Copy link

I am getting this error while using Javers 3.6.3 with SQLServer and 2 JVMs which writes to same database

@bartoszwalacik
Copy link
Member

@a-mujthaba321 start from bumping javers version to latest, and please don't comment on closed issues. Open a new one, if you are sure that you found a bug

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

No branches or pull requests

4 participants