-
Notifications
You must be signed in to change notification settings - Fork 13
Updated Synching Splits to detect if there is a segment in rules #260
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
Conversation
Python 9.1.2 py-yaml vulnerability fix
… sync the segment if it does not exist in storage
… sync the segment if it does not exist in storage
splitio/sync/split.py
Outdated
| self._backoff = Backoff( | ||
| _ON_DEMAND_FETCH_BACKOFF_BASE, | ||
| _ON_DEMAND_FETCH_BACKOFF_MAX_WAIT) | ||
| self.segment_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's create this variable inside the _fetch_until method instead
splitio/sync/split.py
Outdated
| for split in split_changes.get('splits', []): | ||
| if split['status'] == splits.Status.ACTIVE.value: | ||
| # _LOGGER.debug('split details: '+str(split)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be removed
splitio/sync/synchronizer.py
Outdated
| _LOGGER.debug('Starting splits synchronization') | ||
| self._split_synchronizers.split_sync.segment_list = [] | ||
| try: | ||
| self._split_synchronizers.split_sync.synchronize_splits(till) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can set the segment_list here segment_list = self._split_synchronizers.split_sync.synchronize_splits(till)
splitio/sync/segment.py
Outdated
| """ | ||
| if self._segment_storage.get(segment_name) != None: | ||
| return True | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the else is not required, only the return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else: return False is needed to avoid synching segments if the don't exist in sync/split.py _fetch_until() function
splitio/sync/split.py
Outdated
| _ON_DEMAND_FETCH_BACKOFF_MAX_WAIT) | ||
|
|
||
| def _fetch_until(self, fetch_options, till=None): | ||
| def _fetch_until(self, fetch_options, till=None, segment_sync=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
segment sync should not be optional since it's used without checks.
still, i'd suggest not passing around the segment sync so deep and just returning a list of all the segments referenced in the splits. later on, when the segments are indeed being fetched, filter out the ones that are already present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic used to be in sync/synchronizer.py, Matti asked to move it to sync/split.py, that is why segment_sync object is needed. I will put a check before using it to avoid errors.
2- Created test to verify segment_sync is called once from split_sync 3- disable waiting for segment sync workerpool job when synching segments from splits. 4- Other general cleanup.
Python SDK
What did you accomplish?
Updated Synching Splits to detect if there is a segment in rules, and sync the segment if it does not exist in storage
How do we test the changes introduced in this PR?
Updated tests and ran them locally successfully.
Extra Notes
Related JIRA: https://splitio.atlassian.net/browse/SDKS-2374