FoundationDB / fdb-record-layer

A record-oriented store built on FoundationDB
Apache License 2.0
589 stars 104 forks source link

Potential bug on enable Multi-Threaded FDB client in FDBDatabaseFactory #1442

Open mengranwo opened 3 years ago

mengranwo commented 3 years ago

FDB version 6.3 has introduced this new feature called multi-threaded client and seems like the record layer has added support for it. link However, when I was reading through the doc, look like in order to enable the multi-threaded client feature, you must configure at most one external lib.

Warning

In order to use the multi-threaded client feature, you must configure at least one external client. See :ref:`multi-version client API <multi-version-client-api>` for how to configure an external client.

And I have clarified this with the FDB community

Q: Why do I need to configure an external client if both FDB cluster (server-side) and FDB client lib are on the same 6.3 version? I thought multi-version client is mainly used for communicating with FDB clusters that on a different version.
A: It’s now also used for the multi-threaded client feature

The example provided in FDB repo: https://github.com/apple/foundationdb/pull/4339/files#diff-d2d1739d41f9d18cb4b774fb351781cc7c4d3af07ae028e1a2895b69441880f4R72

I guess we will need to configure at least one external client in the code when setting the thread number for FDB client

alecgrieser commented 3 years ago

Yeah, I believe that that's correct. Barring changes from the FDB core side to remove this requirement, the best we can do might be to update the comments on setThreadsPerClientVersion to warn people about the necessary configuration: https://github.com/FoundationDB/fdb-record-layer/blob/233a72a51182a1f35b9bad34ac29b127a437a12b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBDatabaseFactoryImpl.java#L234

I'm adding the documentation label, as I think that's the action item here (at least on this project), but if you think there's some other way the project could handle this, we're open to alternatives