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

LockTree library, originally from PerconaFT #7753

Conversation

spetrunia
Copy link
Contributor

To be used for implementing Range Locking.

@adamretter
Copy link
Collaborator

What are the implications of introducing GPL2 and AGPL3 code into the RocksDB codebase?

@spetrunia
Copy link
Contributor Author

@adamretter It is also licenced under Apache Licence, see utilities/transactions/lock/range/range_tree/lib/COPYING.APACHEv2

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ghost
Copy link

ghost commented Dec 7, 2020

@spetrunia Can we also include updates to Makefile, cmake, and TARGET?

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from 21d3578 to 87c00f9 Compare December 7, 2020 21:57
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ghost
Copy link

ghost commented Dec 8, 2020

@spetrunia compilation in internal repo fails with errors like

stderr: In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/manager.cc:60:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/lock_request.h:58:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/locktree.h:70:
internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/util/omt.h:793:10: fatal error: 'omt.cc' file not found
#include "omt.cc"
         ^~~~~~~~
1 error generated.
Cannot execute a rule out of process. On RE worker. Thread: Thread[main,5,main]
Command failed with exit code 1.

@ghost
Copy link

ghost commented Dec 8, 2020

@spetrunia compilation in internal repo fails with errors like

stderr: In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/manager.cc:60:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/lock_request.h:58:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/locktree.h:70:
internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/util/omt.h:793:10: fatal error: 'omt.cc' file not found
#include "omt.cc"
         ^~~~~~~~
1 error generated.
Cannot execute a rule out of process. On RE worker. Thread: Thread[main,5,main]
Command failed with exit code 1.

we need to include path like utilities/transactions/....../omt.cc

@spetrunia
Copy link
Contributor Author

spetrunia commented Dec 8, 2020

#include "omt.cc"

we need to include path like utilities/transactions/....../omt.cc

This is a bit odd - the library code has plenty of #include "file_in_this_directory.h" . But I've made the change. (This is the only remaining .cc file that is not a compilation unit.. should I rename it to omt_impl.h for the sake of uniformity?)

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from 87c00f9 to b8d17b9 Compare December 8, 2020 22:25
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from b8d17b9 to 74dd0de Compare December 8, 2020 22:32
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from 74dd0de to f1be51f Compare December 8, 2020 22:38
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from f1be51f to 92e4660 Compare December 8, 2020 23:00
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

@ghost
Copy link

ghost commented Dec 8, 2020

#include "omt.cc"

we need to include path like utilities/transactions/....../omt.cc

This is a bit odd - the library code has plenty of #include "file_in_this_directory.h" . But I've made the change. (This is the only remaining .cc file that is not a compilation unit.. should I rename it to omt_impl.h for the sake of uniformity?)

omt.h, concurrent_tree.h, and treenode.h all include their corresponding .cc, all for the template definitions.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ghost
Copy link

ghost commented Dec 8, 2020

#include "omt.cc"

we need to include path like utilities/transactions/....../omt.cc

This is a bit odd - the library code has plenty of #include "file_in_this_directory.h" . But I've made the change. (This is the only remaining .cc file that is not a compilation unit.. should I rename it to omt_impl.h for the sake of uniformity?)

renaming to omt_impl.h sounds good to me.

@ghost
Copy link

ghost commented Dec 9, 2020

internal build is still failing with

stderr: In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/wfg.cc:61:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/locktree/txnid_set.h:55:
In file included from internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/portability/txn_subst.h:8:
internal_repo_rocksdb/repo/utilities/transactions/lock/range/range_tree/lib/util/omt.h:793:10: fatal error: 'utilities/transactions/lock/range/range_tree/lib/util/omt.cc' file not found
#include "utilities/transactions/lock/range/range_tree/lib/util/omt.cc"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Cannot execute a rule out of process. On RE worker. Thread: Thread[main,5,main]
Command failed with exit code 1.

@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from 92e4660 to 24b3d42 Compare December 9, 2020 10:02
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

@spetrunia
Copy link
Contributor Author

spetrunia commented Dec 9, 2020

renaming to omt_impl.h sounds good to me.

Ok, moved.

@spetrunia
Copy link
Contributor Author

Hmm, also, CMakeLists.txt adds some of library's .h files into SOURCES. Let me fix that as well

To be used for implementing Range Locking.
src.mk, CMakeLists.txt and TARGETS are updated to compile
the files from the library.
@spetrunia spetrunia force-pushed the spetrunia-range-locking-rocksdb-pr2-lib-dir branch from 24b3d42 to c6fd639 Compare December 9, 2020 10:41
@facebook-github-bot
Copy link
Contributor

@spetrunia has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Cheng-Chang merged this pull request in 98236fb.

GNU Affero General Public License for more details.

You should have received a copy of the GNU Affero General Public License
along with PerconaFT. If not, see <http://www.gnu.org/licenses/>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some files, e.g. this one, seems to miss Apache license in the header. @spetrunia can we add them?

Copy link

Choose a reason for hiding this comment

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

@siying The license is updated in #7801.

codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
Summary:
To be used for implementing Range Locking.

Pull Request resolved: facebook#7753

Reviewed By: zhichao-cao

Differential Revision: D25378980

Pulled By: cheng-chang

fbshipit-source-id: 801a9c5cd92a84654ca2586b73e8f69001e89320
@ajkr ajkr mentioned this pull request Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants