fix: increase default dataset sync timeout - #109
Conversation
|
20 seconds as a default wait I think it's a crazy long time. I think that we should escalate this to platform, instead of punishing us by waiting (some test run will be shorter than this waiting time!). Also, I don't particularly like the custom option, it's not clear and again, it's not something that we should care about: 10s is a soft "just in case" buffer, it must not be "we have to wait otherwise it does not work". If that's the case, there is something wrong and we need to report it, not accept it and work around it. |
|
Also, if the problem is a crazy fast run, you can retry the test multiple time. It's not a guarantee of success, but the same is for the delay. |
|
The thing is, the platform is just eventually consistent. In the docs it's recommended to wait around 10s minimum. So waiting 20s to be 99.99% sure it's synced doesn't seem like a big deal to me. But we can also leave it at the previous 10s, but expose the option so we can adjust it to our liking in video team 😄
I would of course rather have the platform fix this and be strongly consistent but I don't think that's anything easy on their end to do so. It would take months minimum.
Wdym? The naming or the description isn't?
That's exactly what it is though. We do have to wait because of the platform. |
|
I will open up a thread in product insights though 🤔 |
|
Here is the thread: |
metalwarrior665
left a comment
There was a problem hiding this comment.
20 seconds as a default wait I think it's a crazy long time. I think that we should escalate this to platform, instead of punishing us by waiting (some test run will be shorter than this waiting time!).
Platform won't solve this since this would require complete revamp of the whole backend. They might do it in some future but it is a huge project.
Also, I don't particularly like the custom option
Agree with Luigi, this should not be an option because it is not really a configuration thing, it is required to function.
Also, if the problem is a crazy fast run, you can retry the test multiple time. It's not a guarantee of success, but the same is for the delay.
There is already default 1 retry for tests. That will mostly work but doesn't solve the problem since it can happen for long running scrapes too.
My opinions:
- Default 20 sec delay is ok-ish. Tests run in parallel so in the end this should be only + 10 sec wait. Not great but not terrible.
- I did some clauding and it offered few workaround hacks that would probably be better solution:
a. Load withclean: falseand `fields: ['nonexistent'] which gives you array of empty objects. This still requires the platform to load everything so adds some delay and overhead for very large datasets (rare in tests)
b.Do binary search using limit=1 and offset=N where we set N to known count + some pivot and then either binary up or binary down. This is probably worse delay than just reading all items empty but we could test it.
c. Platform fix I shared to Slack: we should fix X-Apify-Pagination-Total header which can then be used to get real time dataset count. Currently, it is taken from Mongo itemCount but we already have access to DatasetItemsStream that computes the real live count from Redis/S3. If this would be possible fix, we could do limit=1 and then read it from the header.
So 20 seconds default it is then 👍 2.a. and 2.b. are both just complicated workarounds that both just try to accomplish the same thing as increasing the timeout already does. |
|
One thing I forgot in my analysis is that these hacks would only help with dataset items but not with events. So for that we need the delay anyway. |
Turns out that the previous ten seconds are also not enough in some rare cases. Increasing to 20s should fix this.
I've decided to also make this option modifiable by the user, but I'm not quite sure about passing it down the arguments of the functions like this. Let me know your thoughts.
Fixes https://github.com/apify-store/amazon/issues/687