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

Invalid next CommitId resolving algorithm? #221

Closed
floresek opened this issue Oct 8, 2015 · 5 comments
Closed

Invalid next CommitId resolving algorithm? #221

floresek opened this issue Oct 8, 2015 · 5 comments
Assignees
Labels

Comments

@floresek
Copy link

@floresek floresek commented Oct 8, 2015

Hi,

I am evaluating javers in some project and noticed (several times) an blocking problem with resolving next CommitId.

Current implementation resolving next commit id basing on max(commit_pk) record commit_id column value (increasing major nr by 1 and giving next minor from a pool).

In mentioned above cases max(commit_pk) record have not got max(commit_id) value from existng records.
There were records with less commit_pk and bigger commit_id (including 'next' commit id value).
In such situation no commit was possible to perform - javers thrown exception:

can't save already persisted commit

The only way to restore javers to work was to manually update record in JV_COMMIT table.

Because of lack because of lack standard functions to compare version string in standard SQL (which could be used in PolyDB query) I prepared a workaround as in following code:

package org.javers.repository.sql.reposiotries;

public class CommitMetadataRepository {

public CommitId getCommitHeadId() {
// commented current implementation
//        Optional<Long> maxPrimaryKey = selectMaxCommitPrimaryKey();
//        return maxPrimaryKey.isEmpty() ? null : selectCommitId(maxPrimaryKey.get());
        return selectMaxCommitId();
    }

    private CommitId selectMaxCommitId() {
        // FIXME because of lack standard functions to compare version string in standard sql I have to parse some recent records
        SelectQuery query = polyJDBC.query()
                .select(String.format("%s, %s", COMMIT_PK, COMMIT_COMMIT_ID))
                .from(COMMIT_TABLE_NAME)
                .orderBy(COMMIT_PK, Order.DESC)
                .limit(100);

        List<CommitId> commitIds = polyJDBC.queryRunner().queryList(query, new ObjectMapper<CommitId>() {
            @Override
            public CommitId createObject(ResultSet resultSet) throws SQLException {
                return jsonConverter.fromJson(resultSet.getString(COMMIT_COMMIT_ID), CommitId.class);
            }
        });

        if (commitIds.size() == 0) {
            return null;
        }

        if (commitIds.size() > 1) {
            Collections.sort(commitIds, new Comparator<CommitId>() {
                @Override
                public int compare(CommitId o1, CommitId o2) {
                    return -o1.compareTo(o2);
                }
            });
        }

        return commitIds.get(0);
    }

}

I hope you can prepare better code (as authors of PolyDB) resolving the problem ;)
Could you refer to this problem, please?

Cheers
Mariusz

P.S. I registered an issue in PolyDB project:
polyjdbc/polyjdbc#15

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 9, 2015

Hi, in fact I'm not an author of Poly but I'm quite accustomed with this tool. Will take a look at your fix, could you change it to Pull Request from your fork?

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 9, 2015

I'm affraid that select x from y order by z limit 100 means full table scan for most databases, that would badly affect query performance, we need faster query based on db index...

@bartoszwalacik bartoszwalacik added the bug label Oct 9, 2015
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 9, 2015

I think we should convert commit_id column to number(12,2) and add index on it

@bartoszwalacik bartoszwalacik self-assigned this Oct 13, 2015
bartoszwalacik added a commit that referenced this issue Oct 14, 2015
first steps
bartoszwalacik added a commit that referenced this issue Oct 14, 2015
script for MySql
bartoszwalacik added a commit that referenced this issue Oct 14, 2015
scripts for all databases
bartoszwalacik added a commit that referenced this issue Oct 14, 2015
commit_id numeric type
bartoszwalacik added a commit that referenced this issue Oct 14, 2015
#221 numeric commit_id in SQL
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 14, 2015

fixed in release 1.3.16

@floresek
Copy link
Author

@floresek floresek commented Oct 15, 2015

perfect job ;)

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
2 participants
You can’t perform that action at this time.