-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Go binding: add GetClientStatus method to Database #11627
base: main
Are you sure you want to change the base?
Conversation
63c1ccc
to
6e6cc68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, do you think it make sense to have a go struct that can be used to deserialize the result?
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
I was going to say that if we add it, should also be added for the machine-readable JSON status (from key Pros:
Cons:
I can give this a try, and point out fields whose type I am not sure about; what do you prefer? |
Another take at this would be: if we want to support as many versions as possible, we will return the raw values from FoundationDB for all of the special keys (coordinators etc) and never act as a middle-man/translator between client and server. This would extend to the client status information as well. |
@johscheuer any idea why in some cases an empty string and no error can be returned? I am trying to reproduce this, but without success so far. |
39ad203
to
a93fdaa
Compare
Re-based and added a commit which updates the GoDoc for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Thanks for re-triggering the CI; I can check next Friday why it's failing and update this PR. |
@jzhou77 the error seems to be about code formatting (see below); however I have no idea how to address this 🤔 is it due to something I changed in this PR? I have also just re-based the PR, in case it helps.
|
d032506
to
a4f5749
Compare
code format error should be only for ARM build, because we've changed build environment to clang 19. You can ignore these errors. |
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Mention that R/O transactions are garbage-collected once futures go out of scope.
Allow fetching client status JSON information for any database with multi-version client enabled; the raw JSON is returned, so that multiple versions of FoundationDB are supported without any Go structure constraint.
a4f5749
to
cde6c64
Compare
Thanks; now the failure I see is a failed assertion:
So my conclusion is that setting network option causes a 4100 internal error; I removed that from the test and moved it to a new example. |
@jzhou77 can you please open/close to trigger CI? Tests should pass now |
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Add
GetClientStatus
method toDatabase
.Allow fetching client status JSON information for any database.
Added a test to make sure that it works with an open database.
By looking in
fdbclient/MultiVersionTransaction.actor.cpp
I found out thatfdb_database_get_client_status()
will return an empty string when the db is invalid; I will document this in this same PR.Related: #11621
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branch
ormain
if this is the youngest branch)