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

Fixes #6793: Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range #6797

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

nblintao
Copy link
Contributor

@nblintao nblintao commented Apr 7, 2022

Deprecate fdb_transaction_get_range_and_flat_map which was introduced prematurely in 7.0, in preference of fdb_transaction_get_mapped_range in 7.1.

Code-Reviewer Section

The general 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 master 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)

@nblintao nblintao changed the title Fix #6793: Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range Fix Issue#6793: Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range Apr 7, 2022
@nblintao nblintao changed the title Fix Issue#6793: Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range Fixes #6793: Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range Apr 7, 2022
@nblintao nblintao added this to the 7.1 milestone Apr 7, 2022
@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: b373af9
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: da5d2a6
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

jzhou77
jzhou77 previously approved these changes Apr 8, 2022
@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 83cdf9a
  • Result: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)

// function_(ver-1) is the old implementation
//
// WARNING: use caution when implementing removed functions by calling public API functions. This can lead to
// undesired behavior when using the multi-version API. Instead, it is better to have both the removed and public
// functions call an internal implementation function. See fdb_create_database_impl for an example.
FDB_API_CHANGED(fdb_transaction_get_mapped_range, 700);
FDB_API_CHANGED(fdb_transaction_get_mapped_range, 710);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this function is introduced only in 7.1 you don't need to mark is as changed. Changed are functions that existed in previous versions with the same name, but their signature or behaviour have been changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

int iteration,
fdb_bool_t snapshot,
fdb_bool_t reverse) {
fprintf(stderr, "UNIMPLEMENTED FDB API FUNCTION\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we completely remove this function, this can be a problem for upgrade. Imagine an application using this function needs to be upgraded from 7.0.0 to 7.1.0. For that the application will be started with client libraries for 7.0 and 7.1 and FDB_API_VERSION=700. It won't be possible to have a version of an application that is working both with 7.0 and 7.1, because in the 7.0 library, the new function is not available and in 7.1 library the old function is not available. If you want to remove this function, you can remove it only for 7.1.0 i.e. for (#if FDB_API_VERSION >= 710). Also see how other functions were remove in the past. This means that if the 7.1. client library is used in 7.0 compatibility mode (i.e. initialized with fdb_select_api_version(700)), it must find the function providing the same signature and behavior. You have a choice between preservinv the old implementation as it was or reimplement as it as adapter of the new function. In any case, we have to be careful about this, because the CI does not test the upgrade scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. fdb_transaction_get_range_and_flat_map was added prematurely in 7.0, but nobody actually uses and nobody needs it, so we decide to just remove it.

@@ -609,7 +609,7 @@ void DLApi::init() {
headerVersion >= 0);
loadClientFunction(&api->transactionGetRange, lib, fdbCPath, "fdb_transaction_get_range", headerVersion >= 0);
loadClientFunction(
&api->transactionGetMappedRange, lib, fdbCPath, "fdb_transaction_get_mapped_range", headerVersion >= 700);
&api->transactionGetMappedRange, lib, fdbCPath, "fdb_transaction_get_mapped_range", headerVersion >= 710);
loadClientFunction(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also fdb_transaction_get_range_and_flat_map must be loaded, for headerVersion < 710

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same. No longer support fdb_transaction_get_range_and_flat_map in 7.0.

@nblintao nblintao requested a review from jzhou77 April 8, 2022 17:28
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: eb270d4
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@sfc-gh-vgasiunas sfc-gh-vgasiunas merged commit c30caff into apple:main Apr 8, 2022
@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

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.

Upgrade from 7.0.0 to 7.1.0 fails because of missing function fdb_transaction_get_mapped_range
5 participants