https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422018-11-28T16:44:11ZArvadosArvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=692882018-11-28T16:44:11ZTom Cleggtom@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Tom Clegg</i></li><li><strong>Target version</strong> changed from <i>To Be Groomed</i> to <i>2018-12-12 Sprint</i></li></ul> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=693142018-11-28T21:56:09ZTom Cleggtom@curii.com
<ul><li><strong>Category</strong> set to <i>Keep</i></li><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul><p>14538-async-write @ <a class="changeset" title="14538: Check write concurrency limit. Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasge..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/78c18757e42c40178d7a9eaf78f7b6d167bee926">78c18757e42c40178d7a9eaf78f7b6d167bee926</a></p>
<p>This allows up to 4 concurrent writes per file while writing, and up to 4 concurrent writes per collection when syncing a collection in MarshalManifest.</p>
TODO:
<ul>
<li>make the limit configurable</li>
<li>limit concurrency at the filesystem level instead of per-file, to accommodate callers that write multiple large files concurrently</li>
</ul>
<p>I'm not sure whether this should be held up by one or both of those.</p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=693212018-11-29T14:38:46ZTom Cleggtom@curii.com
<ul></ul><p>I'm thinking the default/initial write concurrency limit (flushing finished blocks while writing a file) should be 1 rather than 4. If the Keep side is slower than the incoming data, more concurrency won't necessarily help -- it'll just use more memory.</p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=693362018-11-29T16:42:26ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Over all LGTM assuming tests pass (link to jenkins below). Here's some mostly stylistic comments, take what you will.</p>
<p>In dirnode.sync():</p>
<ul>
<li>Stylistically, I don't know if flush() really benefits from being declared inline. It could just as easily be a method of dirnode and take throttle as a parameter. In order to follow execution of the code, I have to scroll down to the bottom of the function, and then scroll back up again.</li>
</ul>
<ul>
<li><code>pendingLen</code> is used before it is assigned. I know this means it has its default value (0) but it would be helpful to make that explicit so it doesn't look like a bug.</li>
</ul>
<p>In pruneMemSegments():</p>
<ul>
<li>This seems redundant: <code>seg.Len() < maxBlockSize || seg.Len() == 0</code></li>
</ul>
<ul>
<li><del>Is there a potential race between pruneMemSegments() and sync() ? sync() does not check "flushing" so it seems like it might try to flush a block which is already being flushed. By my reading of the code, it probably is not a disaster if this happens (it would replace one storedSegment with an equivalent one) but worth considering.</del> Now I see it calls waitPrune() before that, so no problem.</li>
</ul>
<blockquote>
<p>I'm thinking the default/initial write concurrency limit (flushing finished blocks while writing a file) should be 1 rather than 4. If the Keep side is slower than the incoming data, more concurrency won't necessarily help -- it'll just use more memory.</p>
</blockquote>
<p>I expect the common case is slow clients / fast backend, in which case more threads won't get used. And as you said, fast client / slow backend would result in a pileup. The Python client uses 2 threads by default, which enables it to continue making progress if one write stalls due to a transient failure.</p>
<p>I'm a little concerned that flushing a collection seems to require iterating over every single segment of every single file, which could be expensive for very large collections (some benchmarking is warranted).</p>
<p>I submitted a test run here:</p>
<p><a class="external" href="https://ci.curoverse.com/view/Developer/job/developer-run-tests/986/">https://ci.curoverse.com/view/Developer/job/developer-run-tests/986/</a></p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=693402018-11-29T20:33:58ZTom Cleggtom@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<ul>
<li>Stylistically, I don't know if flush() really benefits from being declared inline. It could just as easily be a method of dirnode and take throttle as a parameter. In order to follow execution of the code, I have to scroll down to the bottom of the function, and then scroll back up again.</li>
</ul>
</blockquote>
<p>Yes, sync() was getting long. Moved flush to (*dirnode)commitBlock(ctx, throttle, []fnSegmentRef).</p>
<blockquote>
<ul>
<li><code>pendingLen</code> is used before it is assigned. I know this means it has its default value (0) but it would be helpful to make that explicit so it doesn't look like a bug.</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>This seems redundant: <code>seg.Len() < maxBlockSize || seg.Len() == 0</code></li>
</ul>
</blockquote>
<p>Indeed. Removed the <code>== 0</code> part.</p>
<blockquote>
<p>I'm a little concerned that flushing a collection seems to require iterating over every single segment of every single file, which could be expensive for very large collections (some benchmarking is warranted).</p>
</blockquote>
<p>This seems unavoidable, since flushing a collection involves producing a manifest which references every segment of every file. Now we iterate twice, but (even without the concurrency being added here) I expect that's still much faster than the ensuing network round-trip with the resulting manifest.</p>
<p>We could stash the last known manifest text and use a "dirty" flag to optimize the no-op case, but it seems out of scope here unless I'm missing something.</p>
<blockquote>
<p>test run</p>
</blockquote>
<p>Fixed flaky test.</p>
<p>14538-async-write @ <a class="changeset" title="14538: Fix racy tests. Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/a88f7ad9728ee6968367928c6d3d7613bbf290ec">a88f7ad9728ee6968367928c6d3d7613bbf290ec</a> <a class="external" href="https://ci.curoverse.com/view/Developer/job/developer-run-tests/988/">https://ci.curoverse.com/view/Developer/job/developer-run-tests/988/</a></p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=694042018-12-03T16:14:49ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Tom Clegg wrote:</p>
<blockquote><blockquote>
<p>I'm a little concerned that flushing a collection seems to require iterating over every single segment of every single file, which could be expensive for very large collections (some benchmarking is warranted).</p>
</blockquote>
<p>This seems unavoidable, since flushing a collection involves producing a manifest which references every segment of every file. Now we iterate twice, but (even without the concurrency being added here) I expect that's still much faster than the ensuing network round-trip with the resulting manifest.</p>
</blockquote>
<p>The Python client is structured a little differently, block operations go through a BlockManager which tracks which blocks are pending, so there's no need to iterate over all the segments when flushing blocks. But this story is about incrementally flushing a PUT of a single large file so it isn't going to be iterating over the manifest except at the end.</p>
<blockquote>
<p>We could stash the last known manifest text and use a "dirty" flag to optimize the no-op case, but it seems out of scope here unless I'm missing something.</p>
<p>14538-async-write @ <a class="changeset" title="14538: Fix racy tests. Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/a88f7ad9728ee6968367928c6d3d7613bbf290ec">a88f7ad9728ee6968367928c6d3d7613bbf290ec</a> <a class="external" href="https://ci.curoverse.com/view/Developer/job/developer-run-tests/988/">https://ci.curoverse.com/view/Developer/job/developer-run-tests/988/</a></p>
</blockquote>
<p>This LGTM.</p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=696682018-12-10T17:18:01ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul><p>Applied in changeset <a class="changeset" title="Merge branch '14538-async-write' fixes #14538 Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/81b8420a261a095a269a46d965b2fc0ee6ecf793">arvados|81b8420a261a095a269a46d965b2fc0ee6ecf793</a>.</p> Arvados - Feature #14538: [keep-web] Do not block writes while flushing blocks to Keephttps://dev.arvados.org/issues/14538?journal_id=719032019-03-01T18:30:19ZTom Morristfmorris@veritasgenetics.com
<ul><li><strong>Release</strong> set to <i>15</i></li></ul>