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

Go binding: add GetClientStatus method to Database #11627

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gm42
Copy link
Collaborator

@gm42 gm42 commented Sep 5, 2024

Add GetClientStatus method to Database.

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 that fdb_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.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

Copy link
Contributor

@johscheuer johscheuer left a 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?

@jzhou77 jzhou77 closed this Sep 6, 2024
@jzhou77 jzhou77 reopened this Sep 6, 2024
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 6e6cc68
  • Duration 0:10:15
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /opt/homebrew/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 6e6cc68
  • Duration 0:11:33
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 6e6cc68
  • Duration 0:25:08
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 6e6cc68
  • Duration 0:30:16
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 6e6cc68
  • Duration 0:36:40
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 6e6cc68
  • Duration 0:40:29
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 6e6cc68
  • Duration 0:45:08
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@gm42
Copy link
Collaborator Author

gm42 commented Sep 9, 2024

Changes LGTM, do you think it make sense to have a go struct that can be used to deserialize the result?

I was going to say that if we add it, should also be added for the machine-readable JSON status (from key \xFF\xFF/status/json), but I just realized that there is no dedicated Go API method to fetch it, so that's not a concern.

Pros:

  • will be gladly welcome downstream

Cons:

  • needs a commitment to be kept up to date every time there is a change
  • if there is a breaking change in such JSON format, the Go binding will need an immediate update because otherwise nobody will be able to read any field from it (even if it's not the one affected by the breaking change)
  • in case of multiple JSON formats for multiple versions, the Go binding will have to forward-port the older versions of the struct to the new version

I can give this a try, and point out fields whose type I am not sure about; what do you prefer?

@gm42
Copy link
Collaborator Author

gm42 commented Sep 9, 2024

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.

@gm42
Copy link
Collaborator Author

gm42 commented Sep 12, 2024

@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.

@gm42 gm42 force-pushed the feat/go-get-client-status branch 2 times, most recently from 39ad203 to a93fdaa Compare September 19, 2024 10:39
@gm42
Copy link
Collaborator Author

gm42 commented Sep 19, 2024

Re-based and added a commit which updates the GoDoc for ReadTransact().

@johscheuer johscheuer closed this Sep 20, 2024
@johscheuer johscheuer reopened this Sep 20, 2024
johscheuer
johscheuer previously approved these changes Sep 20, 2024
Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: a93fdaa
  • Duration 0:21:41
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: a93fdaa
  • Duration 0:32:20
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: a93fdaa
  • Duration 0:40:29
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: a93fdaa
  • Duration 0:42:18
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: a93fdaa
  • Duration 0:45:35
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: a93fdaa
  • Duration 0:49:51
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: a93fdaa
  • Duration 0:54:20
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: a2b7422
  • Duration 0:23:58
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: a2b7422
  • Duration 0:34:14
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: a2b7422
  • Duration 0:41:55
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: a2b7422
  • Duration 0:42:11
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: a2b7422
  • Duration 0:45:42
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: a2b7422
  • Duration 0:49:14
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: a2b7422
  • Duration 0:58:31
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@gm42 gm42 force-pushed the feat/go-get-client-status branch 2 times, most recently from 68e39fa to 59d2a6c Compare October 8, 2024 07:55
@gm42 gm42 closed this Oct 8, 2024
@gm42 gm42 reopened this Oct 8, 2024
@johscheuer johscheuer closed this Oct 8, 2024
@johscheuer johscheuer reopened this Oct 8, 2024
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 59d2a6c
  • Duration 0:21:41
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 59d2a6c
  • Duration 0:33:18
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 59d2a6c
  • Duration 0:40:25
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 59d2a6c
  • Duration 0:43:26
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 59d2a6c
  • Duration 0:44:52
  • Result: ❌ FAILED
  • Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 59d2a6c
  • Duration 0:49:36
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 59d2a6c
  • Duration 0:53:02
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@gm42
Copy link
Collaborator Author

gm42 commented Oct 9, 2024

Thanks for triggering the CI; it is still failing, I think I begin to understand what's happening.

The C client library has logic to seed an internal versionMonitorDb with the first working database connection which supports it, otherwise it falls back on creating one locally.

The get client status function will perform some magic and in one case can return empty string:

ThreadFuture<Standalone<StringRef>> MultiVersionDatabase::getClientStatus() {
	auto stateRef = dbState;
	auto db = stateRef->dbVar->get();
	if (!db.value.isValid()) {
		db.value = stateRef->versionMonitorDb;
	}
	if (!db.value.isValid()) {
		return onMainThread([stateRef] { return Future<Standalone<StringRef>>(stateRef->getClientStatus(""_sr)); });
	} else {

So I believe that for some reason neither databases are considered valid.

@gm42 gm42 force-pushed the feat/go-get-client-status branch 2 times, most recently from 2c09aab to 08d29e0 Compare November 25, 2024 11:23
Mention that R/O transactions are garbage-collected once futures go out of scope.
Allow fetching client status JSON information for any database; the raw JSON
is returned, so that multiple versions of FoundationDB are supported without
any Go structure constraint.

There is no specific API version validation as calling this function will work with any
multi-version client.
@gm42
Copy link
Collaborator Author

gm42 commented Nov 26, 2024

So I believe that for some reason neither databases are considered valid.

The reason turned out to be: multi-version client must be enabled in order for this function call to return non-empty string.

I have updated the PR accordingly; what's left to figure out is whether I can cover both scenarios in tests (MVC enabled/disabled).

Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

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

Successfully merging this pull request may close these issues.

4 participants