Refactoring some code + Added deleteQueue functionality + Added test for the new QueueScheduler #113
Conversation
Update MWS Advanced Object
Issue101 New MWSAdvanced Class
…verwrite one another when created asynchronously.
Update MWS Class + Request Waiting Queue
|
Since the one commit says "start refactoring", should I take it that you intend to add more commits to this? Or should I review as is? |
|
As is. The reason why I named it "start refactoring" is because I think that Queue.js would benefit from some more restructuring. However it would be very hard for me to do so, because I'm not familiar with a lot of choices that were made. Would you be willing to look into that? |
lib/endpoints/Queue.js
Outdated
| @@ -55,13 +55,14 @@ class Queue { | |||
| action, | |||
| maxInFlight, | |||
| restoreRate, | |||
| }) { | |||
| }, deleteQueueFromSchedule) { | |||
There was a problem hiding this comment.
What do you think if we had Queue extend EventEmitter, and emit an 'empty' event that the QueueScheduler listens for in register(), rather than passing in a new parameter here?
could probably do same thing with QueueItem . . . might make more sense overall than passing around function parameters, as much as we do love passing around functions in modern JS :-D
There was a problem hiding this comment.
I'm a huge fan of decoupling code and I'm definitely not a fan of passing functions around as it clutters the arguments of the functions to which they are passed. I also agree with you that it would be nice that when we apply this pattern, that we also apply it to QueueItems.
There was a problem hiding this comment.
I'm very curious, what do you think about delineating the classes even further in, for example, a QueueScheduler, Queue and QueueItem?
There was a problem hiding this comment.
well, let's do that EventEmitter then. And isn't that what the current patch changes the classes to, having 3?
There was a problem hiding this comment.
Well it's a start, but the classes are not yet sufficiently disentangled. I wanted to shoot this idea by you, to see what you think. One of the things that we could do for example is to put the maxInFlight and restoreRate on the QueueScheduler. However, this is hard for me to do as I'm not familiar with some of the choices.
To give an example:
if (this.inFlight >= this.maxInFlight * 0.5) {
this.throttle();
}
Why are we multiplying the maxInFlight times 0.5?
I think that you thought very well about these choices (I don't doubt that they are necessary), but it's hard for someone not familiar with your reasoning to understand why these kind of choices were made. Therefore, I'm afraid that if I start working on disentangling everything further, I'll break the whole thing. Hence, I was wondering whether you could take a look at that.
There was a problem hiding this comment.
I don't feel particularly strongly about that one way or the other, wrt which one has the maxInFlight and restoreRate, but it was put there as is, because it's directly associated to the Queue. Queue keyed by sellerId/category/endpointname.
It seemed prudent to assume that we're very likely to hit a throttle point for sure if we fill the queue halfway already. As far as thought about that specific number goes, I'm pretty sure that i was still seeing a whole lot of requests get to Amazon that I didn't expect to, if I fired a few hundred rapidly, so I tweaked it down to assume that if we've reached the halfway point, we're probably going to fill the queue anyway. Of course, there might also have been a fair amount of hitting hourly quota going rather than inflight quota while i was testing it.
I make no claims that it's the best way, only that it does a pretty decent job at not over-spamming the FBA servers.
:)
There was a problem hiding this comment.
I was out for a couple of weeks on a work trip. Did you have it in mind to continue with some of the changes we've talked about here, or do it separately from this PR?
There was a problem hiding this comment.
Hi @ericblade! I didn't get around to doing that. I propose to that in a separate PR. I did work on extending the functionality of the package locally though. However, the code I wrote for doing that is not polished, I could create a new PR for that? Then you can check if you think the code is good enough to be pulled... If not, I'd totally understand.
There was a problem hiding this comment.
always glad to have a look. i've been burning hard on a R&D project that is going to a demo phase this week, so haven't had any time to work on my own apps, or this.
There was a problem hiding this comment.
Don't worry, I know how it is. Good luck with the project!
checkReportComplete() can returns null in case "_DONE_NO_DATA". If you wouldn't check for this case then reportCheck.cancelled will throw an error if this case occurs.
Update reports.js
|
Thank you!! |
|
I promise I'll get to things in a more timely fashion from this point forward! |
I refactored some code, especially related to Queue.js. I think Queue.js would benefit from some more refactoring, becasue for someone not intimately known with the code, the code might be a bit confusing. QueueItem for example, needs different callbacks from the Queue. It would be nice if we delineate the different classes even more. For example, what we could do is:
At this moment, QueueItem depends on three functions in Queue. It would be nice if we were able to reduce the amount of calls to Queue to one
onCompletecall.P.S. I haven't tested this code with API keys, so there might be errors.