Small fixes for mac arm compilation and fix some tests #1905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maxim-morozov
requested review from
alexowens90,
willdealtry and
poodlewars
as code owners
maxim-morozov
commented
Oct 9, 2024
| @@ -256,12 +256,6 @@ inline KeyType get_key_type_for_index_stream(const StreamId &) { | |||
| return KeyType::TABLE_INDEX; | |||
| } | |||
|
|
|||
| template <typename Function> | |||
| auto foreach_key_type(Function&& func) { | |||
| for(int k = 0; k < int(KeyType::UNDEFINED); ++k) { | |||
There was a problem hiding this comment.
It looks like the keytype = 0 is not used and also there is already a function to convert int to KeyType
There was a problem hiding this comment.
Yes, KeyTypes 1 and 2 were part of the original and now entirely superseded purpose or ArcticDB (as node storage for a specific graph strategy) and were never written anywhere.
maxim-morozov
commented
Oct 9, 2024
| @@ -133,9 +133,8 @@ class PrometheusInstance { | |||
| return; | |||
| } | |||
|
|
|||
| metrics.erase({name, labels}); | |||
| metrics_family.erase(it); | |||
There was a problem hiding this comment.
It looks like the remove operation also removes metrics-family though metrics-family might have additional metrics
maxim-morozov
commented
Oct 9, 2024
| // Create a Storage with 32KB map size | ||
| LMDBStorageFactory factory(32ULL * (1ULL << 10), false); | ||
| // Create a Storage with 64KB map size | ||
| LMDBStorageFactory factory(64ULL * (1ULL << 10), false); |
There was a problem hiding this comment.
In the M1 arm environment, the 32KB map throws a map-full exception on allocation. Looks like the minimum is 64KB.
maxim-morozov
commented
Oct 9, 2024
| @@ -14,7 +14,7 @@ namespace arcticdb { | |||
| #ifdef WIN32 | |||
| #define PAUSE _mm_pause() | |||
| #elif defined(__arm__) || defined(__ARM_ARCH) | |||
| #define PAUSE __yield() | |||
| #define PAUSE asm volatile("yield") | |||
There was a problem hiding this comment.
__yield is not supported on Mac arm architecture.
|
Hi @maxim-morozov . Please include the contributor sign-off in the commit message. |
willdealtry
approved these changes
Oct 11, 2024
Thanks @jamesmunro. I've included the contributor sign-off in the commit message as well. |
…axim Morozov maxim.morozov@gmail.com. By including this sign-off line I agree to the terms of the Contributor License Agreement.
…m Morozov maxim.morozov@gmail.com. By including this sign-off line I agree to the terms of the Contributor License Agreement.
…im.morozov@gmail.com. By including this sign-off line I agree to the terms of the Contributor License Agreement
The new compatibility tests are in `tests/compat`. This includes the following: - `old_venv` fixture which allows executing some arctic commands frow within a custom venv. - 2 compatibility tests. One of which showcases an issue with modifying library options of old environments. - Tweak to the Build and Test workflow to run the compat tests only on newer python versions
Previously modifying a library option with a new version (>=v3.0.0) would break old libraries (<v3.0.0) This was because of the storage overrides introduced in v3.0.0. This change moves the logic to modify a library option to C++ and it does the protobuf modification without applying a storage override. This makes storage overrides work well with both old libraries (as demonstrated by now passing compatibility tests) and with new libraries (as confirmed by test_arctic_library_management.py)
- Exposes a `read_unaltered_lib_cfg` method for the `LibraryTool` - Uses it in compatibility modification test to verify that we only change the new fields
When destructing the mongo storage it was possible to destruct the api instance before destructing the pool. Moved the instance from the `MongoStorage` to the `MongoClient` to be kept as a guard around the `mongocxx::pool`. `MongoStorage` didn't need any sensitive mongocxx objects. We also disable mongo tests for 4.5.0 as it contains the problematic segfault.
All versions up to 4.5.0 have segfaults in lmdb destructor. We skip lmdb backend compatibility tests for these versions to avoid flaky tests.
The absolute paths were breaking some Azure conda-forge runners because abspath was prefixing the path with `$SRC_DIR` which did not get expanded on linux.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
What does this implement or fix?
The fix for the compilation for MAC arm architecture that doesn't support
__yieldinstruction. The fix also covers some unit test failures.Any other comments?
Comments inline of the PR with explanations
Checklist
Checklist for code changes...
Signed-Off By Maxim Morozov maxim.morozov@gmail.com. By including this sign-off line I agree to the terms of the Contributor License Agreement