Feature/grant lease in create #213
Conversation
Antzen
left a comment
Antzen
left a comment
There was a problem hiding this comment.
Overall, this looks good to me. I'm just a little concerned that the integration tests don't fully test correctness since Travis has been having issues / ReadWriteTests is still commented out. I think it might be best for us to clarify the testing situation before merging this in.
| return; | ||
| } | ||
| if (children.size() > 0) { | ||
| if (children.size() > 1) { |
There was a problem hiding this comment.
Is this a fix for an unrelated bug?
There was a problem hiding this comment.
It's a fix for a bug.
| RenewLeaseRequestProto req; | ||
| req.set_clientname(client_name); | ||
| RenewLeaseResponseProto res; | ||
| renew_lease(req, res); |
There was a problem hiding this comment.
This looks like a workaround for creating a lease. Do we not have a "helper function" for creating leases? I might be wrong about there being a cleaner way though; just wondering.
|
I think this pr is isolated from the ReadWriteTest failing and I do not understand why this needs to wait before other test is fixed. I |
|
As we discussed in person last week, resolving the ReadWrite issue before introducing these changes would allow for debugging to be easier, so we held back from merging this PR. But it appears to have been fixed (?), and we need to merge what we have at some point anyhow. Let's go ahead and resolve the merge conflicts and then merge. |
No description provided.