Skip to content

Conversation

@at055612
Copy link
Collaborator

@at055612 at055612 commented Nov 5, 2025

Fixes #249
Fixes #267

Two fixes in one PR as both need each other.

at055612 and others added 30 commits March 6, 2025 16:04
There are now essentially three ways of configuring comparators
when creating a Dbi.

**null comparator**
LMDB will use its own comparator & CursorIterable will call down
to mdb_cmp for comparisons between the current cursor key and the
range start/stop key.

**provided comparator**
LMDB will use its own comparator & CursorIterable will use the
provided comparator for comparisons between the current cursor
key and the range start/stop key.

**provided comparator with nativeCb==true**
LMDB will call back to java for all comparator duties.
CursorIterable will use the same provided comparator for
comparisons between the current cursor key and the range
start/stop key.

The methods `getSignedComparator()` and `getUnsignedComparator()`
have been made public so users of this library can access them.
Refactor DbiBuilder and Dbi ctor to use DbiFlagSet.
Replace Env#copy(File, CopyFlags...) with copy(File, CopyFlagSet).
As there is only one flag this should not be a breaking change.

Deprecate Env#txn(Txn, TxnFlags...) as there is now
Env#txn(Txn)
Env#txn(Txn, TxnFlags)
Env#txn(Txn, TxnFlagSet)
Also improve javadoc and refactor some tests to use DbiBuilder.

Some tests are failing.
@benalexau
Copy link
Member

I have run the format plugin to sort out the whitespace and removed the separators for static inner classes (personally I think they improve readability but appreciate they are not everybody's cup of tea). Not a fan of some of the decisions that the formatter is making, but formatting is incredibly subjective so having automated opinionated formatting does make life easier in a shared code base.

We just use the Spotify fmt-maven-plugin which implements the Google Java Style (which is prescriptive and has no configuration options). I used to use PMD and Checkstyle on LmdbJava but I found it problematic because (a) every update would introduce new warnings which had to be suppressed for no apparent benefit and (b) they reflect options which are debatable at best and impeded people who wanted to provide PRs. I think the code formatter's prescriptive and automated approach strikes the right balance.

While this PR adds the Dbi builder, I decided to keep these methods for the really simple cases. The builder can be used for everything else more complicated. Hopefully a reasonable compromise.

  public Dbi<T> openDbi(final byte[] name, final DbiFlagSet dbiFlagSet) {
  }

  public Dbi<T> openDbi(final String name, final DbiFlagSet dbiFlagSet) {
  }

The builders are such a significant ergonomic improvement and given we are targeting 1.0.0 it might be a good opportunity to remove those methods to encourage visibility of the new golden path.

I have removed the ByteUnits dependency as that repo is no longer being maintained and we were only using a fraction of it and only in tests. I have replaced it with our own ByteUnit class and added new overloaded setMapSize methods to Env and Env.Builder to make it easier for users to specify larger sizes.

Excellent.

@at055612
Copy link
Collaborator Author

@benalexau

The builders are such a significant ergonomic improvement and given we are targeting 1.0.0 it might be a good opportunity to remove those methods to encourage visibility of the new golden path.

Originally I had deprecated all of the open( methods so the only route for Dbi creation was the builder, however I then started to think that for the simplest cases where you just want a named db with no special options, it would be worthwhile to keep the simplest of the open methods. i.e.

        env.createDbi()
            .setDbName("foo")
            .withDefaultComparator()
            .setDbiFlags(DbiFlags.MDB_CREATE)
            .open();

vs

env.open("foo", DbiFlags.MDB_CREATE)  // where MDB_CREATE is treated as a DbiFlagSet

The open(byte[], DbiFlagSet) variant can probably just be done via the builder as that seems like a less common use case.
Having this one open(String, DbiFlagSet) method may also reduce the amount of deprecation warnings people will see vs if it was deprecated.

I'm not too bothered either way so happy to go with whatever you decide.

@benalexau
Copy link
Member

        env.createDbi()
            .setDbName("foo")
            .withDefaultComparator()
            .setDbiFlags(DbiFlags.MDB_CREATE)
            .open();

vs

env.open("foo", DbiFlags.MDB_CREATE)  // where MDB_CREATE is treated as a DbiFlagSet

I'm not too bothered either way so happy to go with whatever you decide.

I prefer the builder approach:

  • Explicitness: The builder shows that foo is the database name, a default comparator exists, and multiple flags are available. Compare that to env.open(..), which provides no such indication (DbiFlags.MDB_CREATE looks like a mandatory constant rather than part of a configurable set)
  • Ergonomics: When users encounter unexpected iteration order, they'll recall the comparator configuration. The builder guides them through compatible comparator selection, avoiding confusion about whether comparators exist, where they're configured, or key formatting (eg endianness, string padding, encoding etc)
  • Easier support: Builder-based code in tickets clearly shows the exact configuration and saves a communications round-trip asking for the precise Dbi configuration
  • Clean migration path: Given we are planning the next release to be 1.0.0 anyway, we can reasonably deprecate these ~10-year-old methods

@at055612
Copy link
Collaborator Author

@benalexau Works for me. Builder it is. 👍

@at055612 at055612 merged commit db70f14 into master Nov 17, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants