<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 19.05.2016 10:58, Razvan Becheriu
wrote:<br>
</div>
<blockquote
cite="mid:CAPj=nFtMdqfS0Q0Xc5T813BB3Tqzf=6sf8W1_1bPqM_nDVFc1w@mail.gmail.com"
type="cite">
<div dir="ltr">Please let me know how to proceed.</div>
</blockquote>
Hi Razvan,<br>
I finally managed to complete this cycle of the review and posted my
comments on github. The code seems to be mostly working. There are
issues, but nothing really major that would prevent us from
continuing the work on this. Everything I found is fixable. And I'm
very, very impressed with the amount of work you put into
unit-tests. I don't say that lightly. I think this is the best
unit-tested patch ISC has ever received. So my overall impression is
that you did a great work! Thank you very much for this.<br>
<br>
I'm not sure what's your previous experience with code reviews is.
At ISC it is common for the review to be very detailed. Some may
even say nit-picking. That's why we often try to split our tasks
into smaller tickets. But that sometimes is not possible, like with
the Cassandra code here. So I went over all my comments and listed 5
that I consider the most essential. Just to recap, these are:<br>
<ol>
<li>
rename dscsql to cass or cassandra (this is important, because
it would eliminate nasty conflicts if we delay this rename)</li>
<li>
move TERASTREAM specific code to separate file or files (we
can't have disabled code or customer specific code in out public
repo. Also, moving the code to separate files will make long
term maintenance a lot easier for you).<br>
</li>
<li>
solve the client-id issue in unit-tests (this looks like an
issue in the LeaseMgr. There's marginal chance that the issue is
with tests, but it's very unlikely, because those tests are
passing for 3 other backends).</li>
<li>
solve the preferred-lifetime issue in unit-tests</li>
<li>
write at least 2 missing unit-tests (for init and version) for
kea-admin. These are essential, because every user in
Kea+Cassandra will use them.<br>
</li>
</ol>
<p>Once those are addressed, I think we would be able to merge the
code to Kea master. Obviously, this will be far from being done
and much more work will be needed, but we can do that in separate
tickets/branches/pull requests. Also, ISC engineers will help with
some of the tasks. Our timeline for Kea 1.1 is very tight, but we
will do what we can. For a superb feature like this we can
probably bend some schedules. :)<br>
</p>
<p>I have pushed couple minor fixes (and soft-wipe functionality
which improves test execution time significantly) to github21
branch.<br>
</p>
<p>Thanks a lot for your work on this.<br>
</p>
<p>Tomek<br>
</p>
</body>
</html>