Last update on this thread was over a week ago. Jenna was kind enough to squash in my reviews (which were essentially provided to her as a patch on top of her patch via GitHub). She has updated the ReviewBoard entries, and I have commented with my +1.
-- Christopher L Tubbs II http://gravatar.com/ctubbsii On Wed, Nov 26, 2014 at 4:12 PM, Christopher <[email protected]> wrote: > On Wed, Nov 26, 2014 at 3:07 PM, Sean Busbey <[email protected]> wrote: > >> I will be as timely as I can. >> >> Thanks. However, please understand that if you are unable to respond > timely, progress can/should/will proceed without you. I just want to make > sure you have opportunity. > > >> Please keep in mind that our community is volunteer based, people have a >> multitude of commitments, and it is a holiday week. >> >> > Sure, I just want to make sure you have ample time to review and we can > address any objections you may have (if you still have any), but I want > some time constraint, so these contributions don't sit idle indefinitely > (that's not fair to the contributor, or to those who'd like to leverage > these features). As long as the discussion is active (holidays/weekends > considered, of course), I will not attempt to push. > > >> I had presumed more feedback wasn't pressing since no one had mentioned on >> the tickets how my original concerns were addressed by the revamps. If >> someone could give me a summary on each it would make things go much >> faster. >> >> > Your feedback is pressing because you're the only one who expressed > concerns, and I want to make sure you have ample opportunity to be > involved. But, I don't want it to wait forever. I did believe I had > responded sufficiently to your questions/concerns... but if there's still > something outstanding that I have not answered or addressed, please raise > it again, explicitly, and in the context of the proposed changes, so I can > respond to it. > > Sure, I can give you a summary as I understand it: > > Overview of the issues: > > ACCUMULO-3177: Adds a per-table VolumeChooser to replace the > RandomVolumeChooser as the default VolumeChooser, with the default > per-table implementation (a new per-table property is added for this) is > now the RandomVolumeChooser. This is modeled after the per-table balancer. > The intent is to enable balancing decisions for a tablet's persistent > storage across volumes in the same way we allow balancing tablet's > "compute" operations across Tablet Servers. To do this, a > "VolumeChooserEnvironment" object is created and added to the chooser API, > which exposes the environment to the chooser (such as the tableId, from > which the namespace can be retrieved, etc.) > > ACCUMULO-3178: Adds an additional example/reference implementation for a > per-table VolumeChooser, called a "PreferredVolumeChooser", which allows > users to specify a specific volume (or set of volumes) in a custom > per-table property. This is used as a test case (integration test) for > ACCUMULO-3177. (Note: your original concerns about deduplication of HDFS's > HSM effort expressed in ACCUMULO-3089 seem to apply only to this issue, not > to ACCUMULO-3177; however, my hope is that you'll see that this is > sufficiently unrelated to that, that your original objections do not apply.) > > Also, some background (from my perspective): > > Recall: As explained on ACCUMULO-3089, your original concerns were > addressed by splitting the original issue into separate tasks, to help > clarify the scope and intentions of that issue with more narrowly defined > changesets. You were requested to explain your objections specifically in > the context of the changesets on those tasks, because I believed you to > have misunderstood the scope of the issue (as I stated on ACCUMULO-3089), > and I hoped that doing so would help clarify where your objections were, in > relation to the proposed code changes. As far as I can tell, you did do > that, with your comments on the subsequent issues. > > You commented on ACCUMULO-3177, suggesting that it might be better to > apply the configuration property on a namespace instead of a table. I > responded to that comment with an explanation of how table and namespace > configurations work in this context. As such, I hold that your comment > represents a recommended "best practice" for table management > (specifically, limiting the granting of ALTER_TABLE), but not an objection > to the table/namespace configuration-based implementation/addition itself. > > You have also commented on ACCUMULO-3178, with an indication that you did > not feel that the VolumeChooser interface itself was strictly "public API", > which I took to mean that you'd be more flexible in tolerating changes to > that API with a higher degree of risk. I did not see an objection to > ACCUMULO-3178, but it does depend on ACCUMULO-3177. You also suggested not > making these dependent on ACCUMULO-3176 (because of the presence of a > limited workaround by adding configuration to a namespace first), a request > which was accommodated (but ultimately unnecessary if ACCUMULO-3176 is > permitted) in a separate review which you did not comment on after > requesting it to be made. > > > > > >> -- >> Sean >> On Nov 26, 2014 12:27 PM, "Christopher" <[email protected]> wrote: >> >> > Thank you for your response. I request that you please be timely, as >> these >> > issues have sat idle for a long time, and the maintenance burden of >> keeping >> > them current with the HEAD of master is becoming excessive. I await your >> > reviews/objections. Thanks. >> > >> > >> > -- >> > Christopher L Tubbs II >> > http://gravatar.com/ctubbsii >> > >> > On Wed, Nov 26, 2014 at 11:30 AM, Sean Busbey <[email protected]> >> wrote: >> > >> > > I will follow up on both of those tickets with my objections, please >> do >> > not >> > > apply them. >> > > >> > > On Tue, Nov 25, 2014 at 2:08 PM, Christopher <[email protected]> >> > wrote: >> > > >> > > > My intention is to, sometime this week, take a thorough look at the >> > > patches >> > > > and reviews available for ACCUMULO-3177 (create a per-table >> > > VolumeChooser) >> > > > and ACCUMULO-3178 (example implementation of an alternate >> > > > VolumeChooser/test case for ACCUMULO-3177). Given the discussions >> > > > surrounding ACCUMULO-3176, and because they originated as the >> overall >> > > > ACCUMULO-3089 (which had some objections that may not have been >> > > resolved) I >> > > > wanted to give this notice, to ensure that there is opportunity for >> > > > objections to occur and be discussed here first. >> > > > >> > > > Presumably, the objections for ACCUMULO-3176 are different from any >> > that >> > > > might be held for 3177 and 3178, but there was limited follow-up >> > > > clarification of the objections raised after the issues were >> re-scoped >> > as >> > > > separate, more specific, JIRA sub-tasks. So, I'm not sure there are >> any >> > > > still outstanding for those. Since the reviews are still open for >> > those, >> > > I >> > > > just wanted to invite discussion either here, or (perhaps even >> better) >> > in >> > > > the specific review in ReviewBoard. >> > > > >> > > > -- >> > > > Christopher L Tubbs II >> > > > http://gravatar.com/ctubbsii >> > > > >> > > >> > > >> > > >> > > -- >> > > Sean >> > > >> > >> > >
