Only read the commit log once during bootstrapping

[ryanhall07]

What this PR does / why we need it:
A recent refactoring of cold writes ( #2508) introduced a regression that increases the
chances the commit log is read twice while bootstrapping. The referenced PR changed the commitlog
bootstrapper to read all requested time ranges, even if a range had been fulfilled by a previous
bootstrapper. This was necessary since the commitlog may have cold writes that were never commmited
to a fileset. The fileystem bootstrapper would report a time range as fulfilled, but might be missing
cold writes only in the commit log.

It should be noted this bug was always theoretically possible, but unlikely since the commitlog bootstrapper
typically wouldn't run in the first pass (cold time ranges) since the filesystem would fulfill all cold ranges
and short circuit the first pass of the boostrapper.

This change only reads the commit log on the first pass of the boostrapper and caches the result to skip reading
it in subsequent passes. It doesn't actually matter which pass we read the commit log, the first was just chosen
arbitrarily.

Other attempts at fixing this bug attempted to disable the entire commit log bootstrapper during a pass, but that's
not possible since the commit log bootstrapper is actually 2 bootstrappers in one, both the the commit log and snapshot
files. To minimize the refactoring changes we still want to only read the snapshot files of the requested ranges.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

[ryanhall07]

unfortunately the diff is not great. I essentially moved all of the commit log reading logic into a new method readCommitLog. additionally I changed the for loop below to iterate over the provided namespaces and not the namespace results from the commit log. didn't seem necessary to use the results in the for loop and it allowed the results to be private in the new method.

[notbdu]

Changes look good so far. Just needs some tests for ensuring that the commit log bootstrapper only bootstraps commit logs once and maybe something additional to ensure snapshots aren't being read twice.

[notbdu]

nit: we could move up the nesting of this logic so we don't need to perform the same check twice.

[ryanhall07]

i don't think we can, since you're already inside a conditional branch for IndexOptions

[notbdu]

nit: It's a little awkward to set it here and use the set value as the return value. We also use the return value after the function call. Maybe it's better to either have no return or return the value and let the caller do the setting?

[ryanhall07]

yea I don't have a strong opinion about this since it's all private local state anyways. I'll just update it so the main function controls the caching logic and decides if it should actually call readCommitLog

[notbdu]

Nice, we should also add this check in the integration tests. Perhaps just to the test in src/dbnode/integration/commitlog_bootstrap_test.go.

[notbdu]

Besides one remaining nit (imports), PR LGTM.

[notbdu]

nit: imports, this goes with the third party imports

[ryanhall07]

verified the change locally with the m3stack

before the change

(rc:rc-05)rhall@ryans-mbp ~ % docker logs m3_stack_m3db_seed_1 2>&1 | grep "read commit logs start"
{"level":"info","ts":1600385779.7967737,"msg":"read commit logs start","bootstrapper":"commitlog"}
{"level":"info","ts":1600385779.825805,"msg":"read commit logs start","bootstrapper":"commitlog"}

after the change

(rc:rc-05)rhall@ryans-mbp ~ % docker logs m3_stack_m3db_seed_1 2>&1 | grep "read commit logs start"
{"level":"info","ts":1600385228.9855318,"msg":"read commit logs start","bootstrapper":"commitlog"}