https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-07-31T17:13:49ZArvadosArvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=129632014-07-31T17:13:49ZWard Vandewegeward@curii.com
<ul><li><strong>Target version</strong> set to <i>2014-08-27 Sprint</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=129782014-07-31T17:18:12ZWard Vandewegeward@curii.com
<ul><li><strong>Subject</strong> changed from <i>Store desired replication level in collection</i> to <i>[API] Store desired replication level in collection</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=129822014-07-31T17:18:48ZWard Vandewegeward@curii.com
<ul><li><strong>Story points</strong> set to <i>1.0</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=132362014-08-06T15:54:01ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Peter Amstutz</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=132432014-08-06T16:02:55ZTim Piercetwp@curoverse.com
<ul><li><strong>Category</strong> set to <i>API</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=132442014-08-06T16:03:50ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>2014-08-27 Sprint</i> to <i>Arvados Future Sprints</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=149172014-09-16T11:37:10ZTom Cleggtom@curii.com
<ul><li><strong>Subject</strong> changed from <i>[API] Store desired replication level in collection</i> to <i>[API] Store desired replication level in collections table</i></li><li><strong>Description</strong> updated (<a title="View differences" href="/journals/14917/diff?detail_id=13634">diff</a>)</li><li><strong>Assigned To</strong> deleted (<del><i>Peter Amstutz</i></del>)</li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=203292015-01-22T14:43:46ZWard Vandewegeward@curii.com
<ul><li><strong>Target version</strong> changed from <i>Arvados Future Sprints</i> to <i>2015-02-18 sprint</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=206222015-01-28T21:34:03ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Peter Amstutz</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=206232015-01-28T21:34:59ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>What's the difference between <code>redundancy_confirmed</code> and <code>redundancy_confirmed_at</code> ? If <code>redundancy_confirmed</code> is just a boolean, then only <code>redundancy_confirmed_at</code> is needed?</p> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=206412015-01-28T22:38:24ZTom Cleggtom@curii.com
<ul><li><strong>Subject</strong> changed from <i>[API] Store desired replication level in collections table</i> to <i>[API] Rename collection "desired replication" attributes, add model constraints</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=207022015-01-29T19:19:20ZTom Cleggtom@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>What's the difference between <code>redundancy_confirmed</code> and <code>redundancy_confirmed_at</code> ? If <code>redundancy_confirmed</code> is just a boolean, then only <code>redundancy_confirmed_at</code> is needed?</p>
</blockquote>
<p><code>replication_confirmed</code> is the number of copies.</p> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=209282015-02-06T19:11:47ZTom Cleggtom@curii.com
<ul><li><strong>Assigned To</strong> changed from <i>Peter Amstutz</i> to <i>Tom Clegg</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=211712015-02-14T20:48:09ZRadhika Chippadaradhika@curoverse.com
<ul></ul><ul>
<li>I am understanding this implementation as follows. For each collection, replication_desired can be set (by admin); or the default will be used. Whatever is this value (default or per collection), we will initiate the replication process when the replication_confirmed != replication_desired. My comments are based on this understanding. Please correct if this is not the case.</li>
</ul>
<ul>
<li>I think the name “replication_current” might have been clearer over “replication_confirmed”.</li>
</ul>
<ul>
<li>when replication_desired is changed, do we need to reset replication_confirmed and replication_confirmed_at? I am thinking this can be set by admin after the initial collection creation and replication_confirmed etc are completed. Or can be updated after setting to a value previously. In either case, I am assuming we would need to replicate additional copies or remove replications as needed. Does this get initiated because replication_desired!= replication_confirmed or do we need to reset these attributes to initiate the replication process?</li>
</ul>
<ul>
<li>maybe_clear_redundancy_confirmed - should this be using the word “replication” in place of “redundancy”?</li>
</ul>
<ul>
<li>ensure_permission_to_save method</li>
</ul>
<p>Why do we want non-admin users be able to clear replication_confirmed* when they cannot set them? Should the following be:</p>
<pre>
if (not current_user.andand.is_admin and
(replication_confirmed_at_changed? or replication_confirmed_changed?) and
not (replication_confirmed_at.nil? and replication_confirmed.nil?))
this:
if (not current_user.andand.is_admin and
(replication_confirmed_at_changed? or replication_confirmed_changed?))
</pre>
<ul>
<li>config/application.default.yml<br />can we update comment — “Set replication level for collections whose replication_desired attribute is nil” => “Default replication level for collections. This will be used when replication_desired attribute is nil.”</li>
</ul>
<ul>
<li>migration script -> comment in up method<br />“Removing that column dropped the index. Let's put it back” => can you please say “search index” instead of “index”?</li>
</ul>
<ul>
<li>test "replication_desired reports #{ask or 'default'} if redundancy is #{ask}" — should this be using the word “redundancy” in here? I think it should be "replication_desired reports #{ask or 'default'} if desired is #{ask}”?</li>
</ul>
<ul>
<li>test "replication_confirmed* cannot be set by non-admin user" do<br />Can you update this comment: “# Cannot set both at once.” => “Cannot set both at once either.”</li>
</ul>
<ul>
<li>test “don't clear replication_confirmed* when just deleting a data block” seems to have done what you intended. Verified as below in the test:</li>
</ul>
<pre>
Initial manifest: . acbd18db4cc2f85cedef654fccc4a4d8+3 37b51d194a7513e45b56f6524f2d51f2+3 0:3:foo 3:6:bar
Updated manifest: . 37b51d194a7513e45b56f6524f2d51f2+3 0:3:foo
</pre>
<ul>
<li>In the fixtures, can we make the names more readable? ex:
<ul>
<li>“replication wantnull havenull” => “replication want-null have-null”</li>
<li>“replication want2 have2” => “replication want-2 have-2” or “replication want 2 and have 2”</li>
</ul></li>
</ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=211772015-02-15T01:41:13ZTom Cleggtom@curii.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote>
<ul>
<li>I am understanding this implementation as follows. For each collection, replication_desired can be set (by admin); or the default will be used. Whatever is this value (default or per collection), we will initiate the replication process when the replication_confirmed != replication_desired. My comments are based on this understanding. Please correct if this is not the case.</li>
</ul>
</blockquote>
<p>The gist of it is that the user specifies a desired replication level, while the data manager gives feedback about the current replication state as often as practicable, and makes additional copies whenever needed. This way the user can check whether the desired level has been achieved, detect when replication falls (even temporarily) below the desired level, and be assured that the data manager process isn't simply stopped or broken.</p>
<p>Given that storage devices and nodes can fail (and return to service) without notice, an assurance about the <em>current</em> replication level is somewhere between impractical and unbelievable. However, a claim about the most recent verification <em>event</em> is practical and believable.</p>
<p>Likewise, the process of comparing desired and actual states has to be done regularly whether or not anyone changes replication_desired on any collections. Data manager could prioritize cases where the latest confirmed level was lower than the current desired level (or was null), though.</p>
<blockquote>
<ul>
<li>I think the name “replication_current” might have been clearer over “replication_confirmed”.</li>
</ul>
</blockquote>
<p>I think "current" implies (incorrectly, see above) that the value represents the state at the time of the <code>collections.get</code> request. Also, "confirmed" highlights the distinction between "specified/desired" (which is always "current") and a confirmation event (which happened in the past). I'm certainly willing to consider other terms ("verified"?), but I do think whatever we choose should match the timestamp field in an obvious way (<code>"replication_current_at"</code> doesn't quite sound right).</p>
<blockquote>
<ul>
<li>when replication_desired is changed, do we need to reset replication_confirmed and replication_confirmed_at? I am thinking this can be set by admin after the initial collection creation and replication_confirmed etc are completed. Or can be updated after setting to a value previously. In either case, I am assuming we would need to replicate additional copies or remove replications as needed. Does this get initiated because replication_desired!= replication_confirmed or do we need to reset these attributes to initiate the replication process?</li>
</ul>
</blockquote>
<p>I think if a user increases replication_desired from 2 to 3, the information about when replication was last confirmed (and whether it was confirmed to be 1 or 2) is still correct, and still just as useful.</p>
<p>It is possible (but not certain) that additional copies of data blocks will be made in order to bring actual replication from 2 to 3.</p>
<blockquote>
<ul>
<li>maybe_clear_redundancy_confirmed - should this be using the word “replication” in place of “redundancy”?</li>
</ul>
</blockquote>
<p>Ah, yes. Fixed in <a class="changeset" title="3410: Rename maybe_clear_redundancy_confirmed to maybe_clear_replication_confirmed." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/08c575dc24bbc5732a5fcb1126c23d9a4ca10b73">08c575d</a>.</p>
<blockquote>
<ul>
<li>ensure_permission_to_save method</li>
</ul>
<p>Why do we want non-admin users be able to clear replication_confirmed* when they cannot set them? Should the following be:</p>
<p>[...]</p>
</blockquote>
<p>I figured this restriction would amount to an inconvenience at most: a non-admin user can achieve the same result by adding an arbitrary data block to the manifest (causing the <code>confirmed</code> fields to reset to nil) and then removing it.</p>
<blockquote>
<ul>
<li>config/application.default.yml<br />can we update comment — “Set replication level for collections whose replication_desired attribute is nil” => “Default replication level for collections. This will be used when replication_desired attribute is nil.”</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>migration script -> comment in up method<br />“Removing that column dropped the index. Let's put it back” => can you please say “search index” instead of “index”?</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>test "replication_desired reports #{ask or 'default'} if redundancy is #{ask}" — should this be using the word “redundancy” in here? I think it should be "replication_desired reports #{ask or 'default'} if desired is #{ask}”?</li>
</ul>
</blockquote>
<p>Hm... I already did this in <a class="changeset" title="3410: Add tests for replication attributes." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/d62b73382398808a440f15fdda2eea2e15e44282">d62b733</a>, which adds a bunch of other tests too. Which version were you looking at?</p>
<blockquote>
<ul>
<li>test "replication_confirmed* cannot be set by non-admin user" do<br />Can you update this comment: “# Cannot set both at once.” => “Cannot set both at once either.”</li>
</ul>
</blockquote>
<p>Done.</p>
<blockquote>
<ul>
<li>test “don't clear replication_confirmed* when just deleting a data block” seems to have done what you intended. Verified as below in the test:</li>
</ul>
<p>[...]</p>
</blockquote>
<p>I don't quite follow you here. I agree that it seems to have done what I intended. (Hopefully it's not unique in this respect?)</p>
<blockquote>
<ul>
<li>In the fixtures, can we make the names more readable? ex:
<ul>
<li>“replication wantnull havenull” => “replication want-null have-null”</li>
<li>“replication want2 have2” => “replication want-2 have-2” or “replication want 2 and have 2”</li>
</ul></li>
</ul>
</blockquote>
<p>Changed to want=2, etc.</p>
<p>Now at <a class="changeset" title="3410: Update comments." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/1f7a6b50cabab4c8645dd6db92e456c080f2a81c">1f7a6b5</a></p> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=211802015-02-15T04:57:23ZRadhika Chippadaradhika@curoverse.com
<ul></ul><ul>
<li>I understand replication strategy better after your first response above. Also agree with your comments about "confirmed" versus "current."</li>
</ul>
<ul>
<li>I was looking at d302307. And, I missed the sdk updates due to this. A couple more comments now that I looked at them (below).</li>
</ul>
<ul>
<li>commands/put.py
<ul>
<li>This comment is still a bit confusing to me.<br />“args.replication is how many copies we instruct Arvados to maintain (by passing it in collections().create() after all data is written).” Are you saying “args.replication is how many copies we instruct Arvados to maintain after all data is written (by passing it in collections().create()).”?</li>
<li>This comment can be a bit more elaborate?<br />“If args.replication is given as None, it should stay None, but we still need to write a suitable number of copies to Keep.” Something like “If args.replication is given as None, replication_desired would be none. However, we will write a default number of copies to Keep …” ?</li>
</ul></li>
</ul>
<ul>
<li>Is this still staying in put.py?<br />if api_client._schema.schemas['Collection']['properties'].get(replication_attr, None) is None:
# API calls it 'redundancy' until <a class="issue tracker-6 status-3 priority-4 priority-default closed parent" title="Idea: [API] Rename collection "desired replication" attributes, add model constraints (Resolved)" href="https://dev.arvados.org/issues/3410">#3410</a>.<br /> replication_attr = 'redundancy'</li>
</ul>
<ul>
<li>Regarding test “don't clear replication_confirmed* when just deleting a data block”<br />You had a comment in the test “We really deleted a block there, right?” I was responding to that comment with my observation that the test is indeed doing what it intends to do.</li>
</ul>
<ul>
<li>I did notice that you had not yet merged latest master (and full text search). I am glad you merged and took care of updating that index as well.</li>
</ul>
<ul>
<li>ensure_permission_to_save<br />Ah. I see the problem now. If we do what I suggested the scenario "clear replication_confirmed* when introducing a new block in manifest" fails which is not acceptable.</li>
</ul>
<ul>
<li>I noticed this issue with the db migration script. When we rollback, the redundancy_confirmed_by_client_uuid is added back and is the last column in the table. Hence the search_index needs to list it as the last column.<br /><pre>
[ 24/260] ArvadosModelTest#test_search_index_exists_on_models_that_go_into_projects = 0.05 s
1) Failure:
ArvadosModelTest#test_search_index_exists_on_models_that_go_into_projects [/home/radhika/arvados/services/api/test/unit/arvados_model_test.rb:144]:
collections has no search index with columns ["owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid",
"portable_data_hash", "uuid", "name", "file_names", "redundancy_confirmed_by_client_uuid"].
Instead found search index with columns ["owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid",
"portable_data_hash", "redundancy_confirmed_by_client_uuid", "uuid", "name", "file_names"]
</pre></li>
</ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=211992015-02-16T08:28:39ZTom Cleggtom@curii.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote>
<p>“args.replication is how many copies we instruct Arvados to maintain (by passing it in collections().create() after all data is written).” Are you saying “args.replication is how many copies we instruct Arvados to maintain after all data is written (by passing it in collections().create()).”?</p>
</blockquote>
<p>Changed to:</p>
<pre><code class="python syntaxhl"> <span class="c1"># write_copies diverges from args.replication here.
</span> <span class="c1"># args.replication is how many copies we will instruct Arvados to
</span> <span class="c1"># maintain (by passing it in collections().create()) after all
</span> <span class="c1"># data is written -- and if None was given, we'll use None there.
</span> <span class="c1"># Meanwhile, write_copies is how many copies of each data block we
</span> <span class="c1"># write to Keep, which has to be a number.
</span> <span class="c1">#
</span> <span class="c1"># If we simply changed args.replication from None to a default
</span> <span class="c1"># here, we'd end up erroneously passing the default replication
</span> <span class="c1"># level (instead of None) to collections().create().
</span></code></pre>
<blockquote>
<ul>
<li>Is this still staying in put.py?<br />if api_client._schema.schemas['Collection']['properties'].get(replication_attr, None) is None:<br /><pre># API calls it 'redundancy' until #3410.
replication_attr = 'redundancy'</pre></li>
</ul>
</blockquote>
<p>Yes, at least for a little while: it allows new Python clients to work against old API servers, e.g., a compute node or shell VM updates to this version before its API server does. I updated the comment from "calls" to "called", though, since this <em>is</em> <a class="issue tracker-6 status-3 priority-4 priority-default closed parent" title="Idea: [API] Rename collection "desired replication" attributes, add model constraints (Resolved)" href="https://dev.arvados.org/issues/3410">#3410</a>...</p>
<blockquote>
<ul>
<li>Regarding test “don't clear replication_confirmed* when just deleting a data block”<br />You had a comment in the test “We really deleted a block there, right?” I was responding to that comment with my observation that the test is indeed doing what it intends to do.</li>
</ul>
</blockquote>
<p>Ah, I see. I meant it as "this is what the next line does" but it can just as well scan as a question to amuse the reader. I've rephrased it:</p>
<pre><code class="diff syntaxhl"><span class="gd">- # We really deleted a block there, right?
</span><span class="gi">+
+ # Confirm that we did just remove a block from the manifest (if
+ # not, this test would pass without testing the relevant case):
</span></code></pre>
<blockquote>
<ul>
<li>I noticed this issue with the db migration script. When we rollback, the redundancy_confirmed_by_client_uuid is added back and is the last column in the table. Hence the search_index needs to list it as the last column.<br />[...]</li>
</ul>
</blockquote>
<p>Indeed! Updated the index. Also updated the test to be less sensitive, since (AFAIK) it doesn't actually matter whether the columns are given in the same order in the table and the index -- only that the set of columns is correct. Right?</p>
<p>Now at <a class="changeset" title="3410: Update comments." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/11e1ee67236b1dda5dac5e871ecfedd7de8faccf">11e1ee</a> (!)</p> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=212232015-02-16T17:32:26ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Idea #3410: [API] Rename collection "desired replication" attributes, add model constraintshttps://dev.arvados.org/issues/3410?journal_id=212332015-02-16T21:00:08ZAnonymous
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>67</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:d87717b4ec885059183ef6d7fa6780c343338455.</p>