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().

@gm42 gm42 requested a review from johscheuer September 19, 2024 10:40
@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: d032506
  • Duration 0:21:54
  • 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: d032506
  • Duration 0:35:48
  • 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: d032506
  • Duration 0:38:12
  • 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 on Linux CentOS 7

  • Commit ID: d032506
  • Duration 0:43:36
  • 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-cluster-tests on Linux CentOS 7

  • Commit ID: d032506
  • Duration 0:54:56
  • 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-macos on macOS Ventura 13.x

  • Commit ID: d032506
  • Duration 0:59:19
  • 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)

@gm42
Copy link
Collaborator Author

gm42 commented Dec 18, 2024

Thanks for re-triggering the CI; I can check next Friday why it's failing and update this PR.

@gm42
Copy link
Collaborator Author

gm42 commented Dec 20, 2024

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

CODE FORMAT NOT CLEAN

THE FOLLOWING FILES NEED TO BE FORMATTED

bindings/c/foundationdb/fdb_c.h
bindings/c/test/apitester/TesterTransactionExecutor.cpp
bindings/c/test/mako/admin_server.hpp
bindings/c/test/test.h
bindings/c/test/unit/trace_partial_file_suffix_test.cpp
bindings/flow/IDirectory.h
bindings/flow/fdb_flow.h
contrib/SimpleOpt/include/SimpleOpt/SimpleOpt.h
contrib/md5/md5.c
contrib/rapidjson/rapidjson/document.h
contrib/rapidjson/rapidjson/rapidjson.h
contrib/rapidjson/rapidjson/reader.h
contrib/rapidjson/rapidjson/writer.h
contrib/rapidxml/include/rapidxml/rapidxml.hpp
contrib/sqlite/sqlite3.h
contrib/sqlite/sqliteInt.h
fdbclient/BlobGranuleReader.actor.cpp
fdbclient/FileBackupAgent.actor.cpp
fdbclient/NativeAPI.actor.cpp
fdbclient/include/fdbclient/Atomic.h
fdbclient/include/fdbclient/BackupAgent.actor.h
fdbclient/include/fdbclient/BlobGranuleRequest.actor.h
fdbclient/include/fdbclient/IConfigTransaction.h
fdbclient/include/fdbclient/RandomKeyValueUtils.h
fdbclient/include/fdbclient/SpecialKeySpace.actor.h
fdbclient/include/fdbclient/StorageServerInterface.h
fdbclient/include/fdbclient/TaskBucket.h
fdbclient/include/fdbclient/TransactionLineage.h
fdbclient/sha1/SHA1.cpp
fdbrpc/FlowTests.actor.cpp
fdbrpc/include/fdbrpc/AsyncFileKAIO.actor.h
fdbrpc/include/fdbrpc/LoadBalance.actor.h
fdbrpc/include/fdbrpc/MultiInterface.h
fdbrpc/include/fdbrpc/ReplicationPolicy.h
fdbrpc/libeio/ecb.h
fdbserver/ConfigDatabaseUnitTests.actor.cpp
fdbserver/DDRelocationQueue.actor.cpp
fdbserver/DiskQueue.actor.cpp
fdbserver/KeyValueStoreRocksDB.actor.cpp
fdbserver/KeyValueStoreSQLite.actor.cpp
fdbserver/KeyValueStoreShardedRocksDB.actor.cpp
fdbserver/RocksDBCheckpointUtils.actor.cpp
fdbserver/VersionedBTree.actor.cpp
fdbserver/include/fdbserver/BulkDumpUtil.actor.h
fdbserver/include/fdbserver/CoordinationInterface.h
fdbserver/include/fdbserver/DDTxnProcessor.h
fdbserver/include/fdbserver/DataDistribution.actor.h
fdbserver/include/fdbserver/RemoteIKeyValueStore.actor.h
fdbserver/include/fdbserver/RestoreCommon.actor.h
fdbserver/include/fdbserver/RestoreController.actor.h
fdbserver/include/fdbserver/RestoreLoader.actor.h
fdbserver/include/fdbserver/RestoreRoleCommon.actor.h
fdbserver/include/fdbserver/RestoreWorkerInterface.actor.h
fdbserver/include/fdbserver/RoleLineage.actor.h
fdbserver/include/fdbserver/TestTLogServer.actor.h
fdbserver/include/fdbserver/workloads/workloads.actor.h
fdbserver/workloads/AsyncFileRead.actor.cpp
fdbserver/workloads/PrivateEndpoints.actor.cpp
fdbservice/FDBService.cpp
flow/Net2.actor.cpp
flow/Platform.actor.cpp
flow/Trace.cpp
flow/include/flow/Buggify.h
flow/include/flow/CodeProbe.h
flow/include/flow/Error.h
flow/include/flow/IndexedSet.h
flow/include/flow/Optional.h
flow/include/flow/TDMetric.actor.h
flow/include/flow/ThreadHelper.actor.h
flow/include/flow/ThreadPrimitives.h
flow/include/flow/Trace.h
flow/include/flow/Traceable.h
flow/include/flow/WipedString.h
flow/include/flow/ppc-opcode.h
flow/include/flow/singleton.h
flow/include/flow/sse2neon.h
flow/include/flow/xxhash.h

@gm42 gm42 force-pushed the feat/go-get-client-status branch from d032506 to a4f5749 Compare December 20, 2024 08:25
@jzhou77 jzhou77 closed this Dec 20, 2024
@jzhou77 jzhou77 reopened this Dec 20, 2024
@jzhou77
Copy link
Contributor

jzhou77 commented Dec 20, 2024

code format error should be only for ARM build, because we've changed build environment to clang 19. You can ignore these errors.

@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: a4f5749
  • Duration 0:22:03
  • 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: a4f5749
  • Duration 0:35:10
  • 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: a4f5749
  • Duration 0:43:17
  • 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-clang-arm on Linux CentOS 7

  • Commit ID: a4f5749
  • Duration 0:49:05
  • 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-cluster-tests on Linux CentOS 7

  • Commit ID: a4f5749
  • Duration 0:57:46
  • 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 added 3 commits December 23, 2024 10:43
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.
@gm42 gm42 force-pushed the feat/go-get-client-status branch from a4f5749 to cde6c64 Compare December 23, 2024 09:46
@gm42
Copy link
Collaborator Author

gm42 commented Dec 23, 2024

Thanks; now the failure I see is a failed assertion:

56/73 Test #71: fdb_go_test .........................................................***Failed    3.66 sec
Database created
setOne called with:  fdb.Database
getOne called with: fdb.Database
Assertion !networkStartSetup failed @ /codebuild/output/src2081500999/src/github.com/apple/foundationdb/fdbclient/MultiVersionTransaction.actor.cpp 3021:
  addr2line -e libfdb_c.so-debug -p -C -f -i 0x13d06ac 0x94b7ae 0x8221bb 0xffff8064b439fd07 0xffff8064b42f4804
--- FAIL: TestGetClientStatus (0.00s)
    fdb_test.go:397: FoundationDB error code 4100 (An internal error occurred)
FAIL

libfdb_c.so-debug is not available in workspace.zip, by using the available one I can symbolize as:

$ addr2line -e build_output/lib/libfdb_c.so -p -C -f -i 0x13d06ac 0x94b7ae 0x8221bb 0xffff8064b439fd07 0xffff8064b42f4804
internal_error_impl(char const*, char const*, int) at /usr/local/bin/../include/c++/v1/string:1499
 (inlined by) __get_pointer at /usr/local/bin/../include/c++/v1/string:1620
 (inlined by) data at /usr/local/bin/../include/c++/v1/string:1279
 (inlined by) c_str at /usr/local/bin/../include/c++/v1/string:1277
 (inlined by) internal_error_impl at /codebuild/output/src2081500999/src/github.com/apple/foundationdb/flow/Error.cpp:64
MultiVersionApi::setNetworkOptionInternal(FDBNetworkOptions::Option, Optional<StringRef>) at /codebuild/output/src2081500999/src/github.com/apple/foundationdb/fdbclient/MultiVersionTransaction.actor.cpp:?
fdb_network_set_option at /codebuild/output/src2081500999/src/github.com/apple/foundationdb/bindings/c/fdb_c.cpp:154
?? ??:0
?? ??:0

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.

@gm42
Copy link
Collaborator Author

gm42 commented Dec 24, 2024

@jzhou77 can you please open/close to trigger CI? Tests should pass now

@jzhou77 jzhou77 closed this Dec 24, 2024
@jzhou77 jzhou77 reopened this Dec 24, 2024
@foundationdb-ci
Copy link
Contributor

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

  • Commit ID: cde6c64
  • Duration 0:21:10
  • 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: cde6c64
  • 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 on Linux CentOS 7

  • Commit ID: cde6c64
  • Duration 0:44:26
  • 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-clang-arm on Linux CentOS 7

  • Commit ID: cde6c64
  • Duration 0:47:16
  • 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-cluster-tests on Linux CentOS 7

  • Commit ID: cde6c64
  • Duration 1:03:34
  • 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)

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

Successfully merging this pull request may close these issues.

5 participants