Forward pointers implementation #651

dimosped · 2018-11-07T11:41:58Z

Implement 3 globally configured env variables:

ARCTIC_FORWARD_POINTERS=Disabled: VersionStore operates as it used to
ARCTIC_FORWARD_POINTERS=Enabled: VersionStore only updates the version document with the fw pointers to segments.
ARCTIC_FORWARD_POINTERS=Hybrid: Compatibility operation. VersionStore updates both the version document with fw-pointers, and the segments with backwards pointersr to parents/versions. Makes it easy to experiment safely with FW pointers, and switch back to original implementation (disabled) without issues. In this mode, reads prefer the FW pointers (if exist) otherwise falls-back to original backwards pointers based reads.

arctic/store/_ndarray_store.py

yschimke · 2018-11-07T12:44:08Z

arctic/store/_ndarray_store.py

-                                          {'$set': segment,
-                                           '$addToSet': {'parent': version['base_version_id']}},
-                                          upsert=True)
+                    if ARCTIC_FORWARD_POINTERS is FwPointersCfg.DISABLED:


Is this block clearer if written as two optional write operations?, the second collection.update_one is rewritten but essentially the same

I decided to favor some code duplication in favor of minimizing the regression risks, and keep the existing code path (with disabled fw pointers) as similar as possible.

We can properly refactor this once we build confidence in our implementation.
I am adding a todo with your suggestion though.

yschimke · 2018-11-07T12:44:49Z

arctic/store/_ndarray_store.py

-            segments = [x['segment'] for x in collection.find({'symbol': symbol, 'parent': parent_id},
-                                                              projection={'segment': 1},
-                                                              )]
+            segments = [x['segment'] for x in collection.find(spec, projection={'segment': 1})]


worth stripping _id?

handy debug info indeed, added now

yschimke · 2018-11-07T12:50:42Z

arctic/store/_ndarray_store.py

                'segment': {'$lt': to_index}
                }
        if from_index is not None:
            spec['segment']['$gte'] = from_index
        else:
            segment_count = version.get('segment_count', None)

+        # We want to use FW pointers to read data if:
+        #  ARCTIC_FORWARD_POINTERS is currently enabled/hybrid AND last version was written with FW pointers,


Should we consider making enabled use the reverse pointers to read, but then checking that the forward pointers reconcile? As a way of validating for now?

Just an idea, I'm not against this approach, just asking what we want to test during the hybrid mode period.

The written data are being verified upon write, i.e. we count the segments matching the write spec which have been actually written.
https://github.com/dimosped/arctic/blob/forward_pointers_final/arctic/store/_ndarray_store.py#L505

Tbh I am not against this idea, but maybe as a separate option/flag and add it as part of the verification check:
https://github.com/dimosped/arctic/blob/forward_pointers_final/arctic/store/_ndarray_store.py#L505

It would also mean r/w will be slower with an extra step of 1-query collecting N-document IDs.

I actually went ahead and implemented this, but during tests which force a failed check, a nasty bug was uncovered.

If mongo_retry operation fails once, the successfully written segments always remain (it is Arctic's design decision which won't change now). The upserted ids though in subsequent retried, don't include them, breaking the model.

Working on it and will add explicit tests.

yschimke · 2018-11-07T12:51:07Z

Looks great to me, but I don't consider myself qualified to approve

bmoscon · 2018-11-07T14:41:59Z

I dont have an issue with this PR - this is more a general critique - rather than using env variables (we have quite a few of them now, all defined in different files) can we also support a YAML file? the default would be env vars, but if any of the values were also defined in the yaml file, those would take precedence?

yschimke · 2018-11-07T14:50:36Z

Building on @bmoscon point, some of these flags could be configured differently per connection. e.g. in the case of a backwards incompatible flag, a migration script might need to configure two instances with different flag values.

dimosped · 2018-11-07T15:03:03Z

I was also thinking to refactor and create a yaml or "arcitc/config.py" module which holds all configurations.
The rest of the modules will simply import variables from this, and user will have all config in a common file.

dimosped · 2018-11-07T15:08:39Z

maybe we could even have instead in config.py a python dict with default values.

I always prefer plain python dicts over yaml when possible, and given all configs are exposed via env variables, they don't require a code re-release with updated values.

@yschimke @bmoscon @jamesblackburn @willdealtry any objections with the above?

yschimke · 2018-11-07T15:34:52Z

@dimosped no objections, it might be best done as a separate PR and move everything over for consistency.

dimosped · 2018-11-07T15:58:50Z

@yschimke agreed, I will add even more config options in the next PR for the internal async work, so it would be best to refactor then, with the next PR.

dimosped · 2018-11-08T13:34:34Z

thanks to @yschimke suggestion for reconciliation option, @bmoscon @jamesblackburn this was a nasty one over with partial segment writes between mongo_retries:
dimosped@f2bb752

Need to also correct/update the versoins/segments garbage collection, and make it compatible also for versions which were created with forward pointers.

jamesblackburn

Thanks Dimos - tricky change! One thing is to try to be as forwards compatible as possible. IMO new versions of arctic should be able to read either pointer scheme without environment variables being set... Similarly new versions of arctic shouldn’t corrupt existing libraries / writes.

I wonder if we need a version scheme on the library or similar, so we can raise early if the arctic version is too old to read data correctly?

Will need some battle testing!

arctic/_util.py

jamesblackburn · 2018-11-16T03:13:51Z

arctic/store/_ndarray_store.py

+    # If this is the first use of fw-pointers, query for the full list of segment IDs,
+    # or continue by extending the IDs from the previous version
+    if previous_version and FW_POINTERS_KEY not in previous_version:
+        segment_ids = {_id for _id in collection.find({'symbol': symbol, 'parent': version_base_or_id(version)},


Worth asserting here the segment id count is right (as is done in the read path)?

check_written has been updated to make this check both for fw pointers and legacy

also, I modified slightly the query to make it targeted for the version.

@yschimke @jamesblackburn @bmoscon I added a check here.
James has a point, because the check_written is only called for writes, not appends, so it is safer to double check here.

I am now marking this conversation as resolved.

arctic/store/_ndarray_store.py

jamesblackburn · 2018-11-16T03:25:49Z

arctic/store/_ndarray_store.py

+            # Reason is that if only some of the segments might get written, and we re-execute write() via mongo_retry,
+            # the upserted_id(s) won't be a complete list of all segment ObjectIDs
+            shas_to_ids = {x['sha']: x['_id']
+                           for x in collection.find({'symbol': symbol}, projection={'sha': 1, '_id': 1})}


This won’t be covered by an index, will it? Also looks quite expensive as you’re iterating over every chunk for a symbol - and mongo will thrash the WT cache as it loads each into memory...

Should we be using shas instead?

I will check this tomorrow with an explain() and see what happens because _id is quite special, and we have already an index for sha.

If this proves indeed to be expensive, I will consider the switch to shas.

Before sharing the explain() results, some benchmarks vs this:
https://github.com/manahl/arctic/blob/master/arctic/store/_ndarray_store.py#L515-L519

I assume the extra time is only for the transfer of the ObjectIDs and execution is acceptably fast.
The extra ms IMHO in almost a second of total time for the read-query, can be considered negligible.

Will share the explain() findings shortly.

@jamesblackburn I am not certain, but I would expect (NO-EVIDENCE here) MongoDB is smart enough to have a light in-memory structure to maintain the relation of an index to _id values, i.e. not having to iterate and really fetch the documents, simply for fetching the IDs, given you have are hitting only fields which are part of an index.

Do we have a way to evaluate the WT cache-pollution effects ? @rob256 @bmoscon any ideas here ?

Interesting, @jamesblackburn look how the projection changes the index used between the first and the last queries.

This raises some suspicion that WT cache might indeed get polluted.
When only sha is in the projection, it selects the symbol_sha index, while when _id is in the projection, it picks the symbol_hashed index.

Ah...
https://docs.mongodb.com/manual/reference/explain-results/#explain.executionStats.totalDocsExamined

Just noticed,

eqlib._collection.find({'symbol': symbol}, projection={'sha': 1, '_id': 1}).explain()

has a FETCH stage,
while this doesn't

eqlib._collection.find({'symbol': symbol}, projection={'sha': 1, '_id': 0}).explain()

It is quite obvious that @jamesblackburn intuition is correct, we would be polluting the cache, an d it is a good idea to convert to using SHAs instead, or add an index (symbol, sha, _id).

I'd rather use SHA though for obvious reasons

jamesblackburn · 2018-11-16T03:28:26Z

arctic/store/_ndarray_store.py

-                                                 projection={'sha': 1, '_id': 0},
-                                                 )
-                                 ])
+        if ARCTIC_FORWARD_POINTERS is FwPointersCfg.DISABLED:


Again shouldn’t this be based on precious_version, not on the environment variable?

I’m worried that people / code writing to different libraries with different env variables set will leave the versions in a mess?

I have already updated in my last commit this, be based whenever makes sense on the previous version configuration.

jamesblackburn · 2018-11-16T03:39:31Z

arctic/store/version_store.py

                                     )
        return [version["base_version_id"] for version in cursor]

-    def _prune_previous_versions(self, symbol, keep_mins=120, keep_version=None):
+    def _prune_previous_versions(self, symbol, keep_mins=0.01, keep_version=None):


Why are you turning keep_mins down?

This is / was needed to handle replication lags etc. Need to ensure this is larger than the maximum lag, or secondary reads can fail.

ignore this, I have set it for testing the pruning, when the PR reaches its final form, I will revert

I will resolve this conversation once I switch it back.

tests/integration/store/test_version_store.py

dimosped · 2018-11-16T04:11:37Z

@jamesblackburn @yschimke my last commit made this change fully forward/backwards compatible between enabled-fw-pointers/legacy-parent-pointers/hybrid.

Tomorrow I will finalize the pruning, with respect to the last change for compatibility.

I will also address the comments

dimosped · 2018-11-16T04:50:02Z

@jamesblackburn @yschimke forgot to mention that an extra aim was to make this also possible/supported:

user tries out FW pointers in hybrid mode (e.g. with arctic v1.73.0), writes symbolX
then another application which is still in e.g. v1.67,1, can read/write symbolX as we are fully backwards compatible
arcitc v1.73.0 can re-read/re-write symbolX touched by v1.67.1
user switches to FW ENABLED (only forward pointers) and now v1.67.0 of course can no longer read/write
user switches back to FW HYBRID or FW DISABLED and touches symbolX again.
v1.67.0 can read/write symbolX again

yschimke · 2018-11-19T06:41:23Z

LGTM - since this is optional, I suggest landing this and adding the tests as next PR. It's a big PR already.

… with only subset of segments written

…nd being also backwards compatible

…ABLED/DISABLED/HYBRID per write/append/read

…, and raise operation errors upon failure

…in the version metadata

…fixed multiple tests after enabling for all VersionStore tests to run in all three modes for forward pointers

…ng with FW pointers too early, as the version is not inserted yet when pruning happens

…ed in the past than 24h. Added fw pointers in multiple other tests to verify functionality

….Binray comparison with binary

yschimke · 2018-11-27T05:10:13Z

🥳

This reverts commit 6735c04.

dimosped self-assigned this Nov 7, 2018

dimosped requested review from jamesblackburn, yschimke, bmoscon and rob256 November 7, 2018 11:41

dimosped force-pushed the forward_pointers_final branch from 5af7701 to 0917cc1 Compare November 7, 2018 11:48

dimosped requested a review from pablojim November 7, 2018 12:12

yschimke reviewed Nov 7, 2018

View reviewed changes

arctic/store/_ndarray_store.py Outdated Show resolved Hide resolved

yschimke reviewed Nov 7, 2018

View reviewed changes

bmoscon approved these changes Nov 7, 2018

View reviewed changes

yschimke approved these changes Nov 9, 2018

View reviewed changes

jamesblackburn approved these changes Nov 16, 2018

View reviewed changes

dimosped added 5 commits November 22, 2018 04:29

initial commit for fw pointers implementation

1d7a581

Finalized implementation of forward pointers

4f2ded2

incorporated PR comments for fw pointers implementation

3838696

fw pointers: chunks_ids to chunk_ids

eda4842

fix for forward pointers implementation to be robust over mongo_retry…

0fad483

… with only subset of segments written

dimosped added 8 commits November 22, 2018 04:29

minor fix for initialization of ARCTIC_FORWARD_POINTERS

15473b2

added implementation of cleanup/pruning supporting forward pointers a…

a905b6a

…nd being also backwards compatible

updated FW pointers implementation to allow any transition from/to EN…

25f2b08

…ABLED/DISABLED/HYBRID per write/append/read

added comments explaining forward pointers variables and methods

100125d

added check for number of gathered segments when updating fw pointers…

64a38e3

…, and raise operation errors upon failure

forward pointers implementation with SHAs instead of IDs

604d801

completed fully functional implementation of SHA based forward pointers

3fb76bd

pruning compatible with forward pointers enabled/disabled/hybrid

ec4adee

dimosped force-pushed the forward_pointers_final branch from a43e2e8 to ec4adee Compare November 22, 2018 04:35

dimosped added 15 commits November 22, 2018 04:37

don't strip() twice in version str

a16f058

remove unnecessary import

aa2a1bb

set back the pruning timeout to 120

c871cbc

set back the pruning timeout to 120

0dbc7ca

added numerical value of the arctic version used to crate a version, …

c89ac36

…in the version metadata

fixed index check integration test

afe5ad9

fixed multiple bugs with concat_and_write, corrected the pruning and …

3992001

…fixed multiple tests after enabling for all VersionStore tests to run in all three modes for forward pointers

fixed all integration tests for versionstore and fixed bug with pruni…

3e33892

…ng with FW pointers too early, as the version is not inserted yet when pruning happens

fixed the last two broken integration tests

5683373

fix the cleanup logic for forward pointers to retain the chunks creat…

63ff46b

…ed in the past than 24h. Added fw pointers in multiple other tests to verify functionality

moved back in original order the publsh_changes and prune calls

7e0a016

fixed python 3 compatiblility issue with pruning

be0bf42

updated changelog

842579f

fixed Binary(b'aaa') != b'aaa' in Python 3

b491ac5

fixed last remaining failed tests for python 3 related to bson.binary…

77d412a

….Binray comparison with binary

dimosped merged commit 6735c04 into man-group:master Nov 27, 2018

shashank88 added a commit to shashank88/arctic that referenced this pull request Jul 9, 2019

Revert "Forward pointers implementation (man-group#651)"

5e01261

This reverts commit 6735c04.

Forward pointers implementation #651

Forward pointers implementation #651

Conversation

dimosped commented Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimosped Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

yschimke commented Nov 7, 2018

bmoscon commented Nov 7, 2018

yschimke commented Nov 7, 2018

dimosped commented Nov 7, 2018

dimosped commented Nov 7, 2018 • edited Loading

yschimke commented Nov 7, 2018

dimosped commented Nov 7, 2018

dimosped commented Nov 8, 2018 • edited Loading

jamesblackburn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimosped Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

dimosped Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

dimosped Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

dimosped Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimosped commented Nov 16, 2018 • edited Loading

dimosped commented Nov 16, 2018 • edited Loading

yschimke commented Nov 19, 2018

yschimke commented Nov 27, 2018

dimosped commented Nov 7, 2018 •

edited

Loading

dimosped Nov 7, 2018 •

edited

Loading

dimosped commented Nov 7, 2018 •

edited

Loading

dimosped commented Nov 8, 2018 •

edited

Loading

dimosped Nov 16, 2018 •

edited

Loading

dimosped Nov 16, 2018 •

edited

Loading

dimosped Nov 16, 2018 •

edited

Loading

dimosped Nov 16, 2018 •

edited

Loading

dimosped commented Nov 16, 2018 •

edited

Loading

dimosped commented Nov 16, 2018 •

edited

Loading