FB Marketing source - lookback window logic not functioning correctly #3395
Conversation
vitaliizazmic
requested review from
grebessi,
htrueman,
keu,
po3na4skld,
vovavovavovavova,
yevhenii-ldv and
Zirochkaa
and removed request for
sherifnada and
masonwheeler
| if self._state and self._state >= cursor: | ||
| continue | ||
| if self._state: | ||
| buffer_days = getattr(self, 'buffer_days') + 1 if hasattr(self, 'buffer_days') else 0 |
There was a problem hiding this comment.
(nitpick) We can declare buffer_days before the loop, and simply replace if self._state and self._state >= cursor: with if self._state and self._state.subtract(days=buffer_days) >= cursor:. It will look prettier, WDYT?
There was a problem hiding this comment.
Yes, we can move declaring buffer_days from the loop. But we should consider that sometimes self._state is None.
| @@ -154,9 +154,10 @@ def read(self, getter: Callable, params: Mapping[str, Any] = None) -> Iterator: | |||
| """Apply state filter to set of records, update cursor(state) if necessary in the end""" | |||
| params = params or {} | |||
| latest_cursor = None | |||
| buffer_days = getattr(self, 'buffer_days') + 1 if hasattr(self, 'buffer_days') else 0 | |||
There was a problem hiding this comment.
| buffer_days = getattr(self, 'buffer_days') + 1 if hasattr(self, 'buffer_days') else 0 | |
| buffer_days = getattr(self, 'buffer_days', -1) + 1 |
There was a problem hiding this comment.
@keu, I think the logic will be a little wrong with your changes
There was a problem hiding this comment.
@yevhenii-ldv it was a test :) ok, I have updated the suggestions
There was a problem hiding this comment.
@vitaliizazmic I have changed my mind, lets add loop_back=0 to IncrementalStream, and just use normal self.loop_back instead
There was a problem hiding this comment.
the problem with the current approach is that base class assumes implementation of its child classes, which actually should be wise versa
There was a problem hiding this comment.
also my bad, I mean lookback or buffer_days
|
@keu I added requested change, could check it? |
|
/publish connector=connectors/source-facebook-marketing
|
What
Connector didn't pull Insights data from the past 28 days while incremental sync. Insights Streams have loopback logic when it requests data from the API. But additional filter was applied while reading, which caused missing data from the past 28 days. I've added checking to this filter to avoid losing data.
Pre-merge Checklist
Closes #2828