Add checkpoint writer - Githubissues

waldemarnt commented 7 years ago

The goal of this pull request is decrease the amount of I/O operations to the database.

To do that we extracted the write checkpoints responsibility from the events-reader to another module and we make the communication based on events to propagate changes asynchronously. We tried to modify events-reader as little as possible.

The image below represents the before state without any debouncing or change on the writing checkpoint:

The image below represents the after state using the new module that listen to events from updateReschedule:

This change is

[ ] General Comment
Coverage increased (+0.3%) to 84.474% when pulling 4a22a58f0d4c8f92c946980067811d14f9016668 on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.2%) to 84.361% when pulling 2738a1facf4279780015f48f759fad2ea93460fa on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.2%) to 84.361% when pulling c695b927a6ed560077fc5c22401adac9e2f84fdd on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.2%) to 84.338% when pulling 20cc3babc9abcbddb60d41e896c25afbd2f97cc4 on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.03%) to 84.164% when pulling 8df134b7313698624eb355835fa67460e692463b on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.03%) to 84.164% when pulling 1f530b27dc334ead9c80045bd14bb029bea889ad on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
Coverage increased (+0.03%) to 84.164% when pulling bdf284de274c03d7651c51bbed6393b3d2e7d15d on add-checkpoint-writer into cd4ba0f6d7a23181ae9370016460d53a77bb82a6 on develop.
[ ] File: lib/events-reader.js:L260-272
remove this one; it'll be very verbose
[ ] File: lib/checkpoint-writer.js:L1-65
Why not setinterval?
Sure, perfect, setInterval is exactly for that. My fault 💃
[ ] File: lib/checkpoint-writer.js:L1-65
this is a little confusing, since you have 2 variables to help you track one state; would be better if there was one.
If you use setInterval, you won't need all this stopped logic; just save the return value and if it exists you know the loop is running.
We need this boolean to know if the loop is running or not, and we need to have an accessor to change its state because Mocha stops the setInterval after each test case. But we will put an accessor instead of receive it as parameter.
You can use the existence of the interval's return as a boolean in your accessor instead of storing state in another var
[ ] File: lib/events-reader.js:L5-13
might be good to call the file events-reader-checkpoint-writer or something, since there are checkpoints for sse as well
Locally, you can keep calling it checkpointWriter, that's clear in this context.
[ ] File: lib/checkpoint-writer.js:L1-65
<personal preference; feel free to ignore> I'd prefer undefined to null
[ ] File: lib/checkpoint-writer.js:L1-65
We can actually pull out agco-logger now, since we've learned all we will from profiling, and we can get rid of a ton of environment variables. You don't have to do this now though, we could do it some other time if you like, but it's prolonging technical debt.
I agree with you, So, let's use for a while and remove it later.
[ ] File: test/checkpoint-writer.spec.js:L1-59
You're not testing that it debounces writes, since its already going to be doing 1ms cycles right now; try with a larger cycle time and writing like 100 times and see how many times it does write, then write again after the larger cycle time passes, and see if it does write again.
We are, because we are using sinon.useFakeTimers() to control the time. If you take a look into the implementation you will see that we use clock.tick(1) to increment the time in 1 using sinon fakeTimers. This way ensures that we have total control of the time to be able to test.
[ ] File: lib/events-reader-checkpoint-writer.js:L1-66
What do you think about passing writeInterval as parameter? That way we could get rid of writeInterval variable.
[ ] File: lib/events-reader-checkpoint-writer.js:L1-66
Do we really need lodash for this?
[ ] File: lib/events-reader-checkpoint-writer.js:L1-61
you should probably save this in a global variable (like writerLoopIsRunning) so you can tell that the interval is running, and so you can call clearInterval (if you want to) to stop it. Since stop isnt required right now, you dont have to (and shouldnt) write that method now.
[ ] File: lib/events-reader-checkpoint-writer.js:L1-61
If the goal is to stopWriterLoop, then just do the clearInterval(writerLoopIsRunning) then writerLoopIsRunning=undefined
But i dont think you really need this stop method right now.
[ ] File: lib/events-reader.js:L229-247
You should pull some of this internal; 'newCheckpoint' is tightly coupled to the writerloop functionality. Would be better to have a checkpointWriter.debounceCheckpointWrite(checkpoint.id,doc) or something similar
I disagree, I think that sending an event is way more decoupled them directly referencing a method from other file.
I think this is a perfect scenario to use an event driven approach, most of the Node.js communication is done by events and in my point of view there's no reason the to communication between events-reader and checkpointWriter be orchestrated. As the name "events-reader" says it just read events and emmits and event with says "We have a new checkpoint" the listeneres will decide what will do with that checkpoint. If we put a function call in "events-reader" to persist this checkpoint we are adding more responsability to events-reader, the responsability to write data.
[ ] File: lib/events-reader-checkpoint-writer.js:L1-61
You'll want to get rid of as much of this interface as is possible (except if some of it is used for testing). If it's used for testing, give it an _ so people know it's internals.
Yeah, I agree but we need to stub that function :(
[ ] File: lib/events-reader-checkpoint-writer.js:L1-61
You probably want to use parseInt unless you know that intervals work well with decimals...
[ ] File: test/checkpoint-writer.spec.js:L1-103
This seems identical to line 47; i dont know that you're testing very much here. You might want to check that it debounces writes, so you call 500 writes in 1ms and expect 1 write.
Yep it make sense, I'll create a test for this scenario that you described