pranadb
pranadb copied to clipboard
cashapp
Lockdown replication factor #360
Locked down the replication factor by saving it in the DB so that the user cannot change it once the cluster has been created.
@purplefox please review and let me know in case I need to make any changes, thanks!
Hi @mohsalsaleem instead of duplicating the code in checkConstantShards, I think it would be better to abstract out the common logic and use that. E.g. create methods, getConfigProperty and setConfigProperty
Thanks for the feedback, I thought so too. Will update it.
On Thu, 16 Jun 2022 at 10:32 AM Tim Fox @.***> wrote:
Hi @mohsalsaleem https://github.com/mohsalsaleem instead of duplicating the code in checkConstantShards, I think it would be better to abstract out the common logic and use that. E.g. create methods, getConfigProperty and setConfigProperty
— Reply to this email directly, view it on GitHub https://github.com/cashapp/pranadb/pull/407#issuecomment-1157286148, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSANGTT7FBM7THYD3R5EP3VPLDAPANCNFSM5Y4MJBNA . You are receiving this because you were mentioned.Message ID: @.***>
-- Regards, V.H.Mohamed Saleem
I've moved the logic to get and set property into its own methods. I've also created a common method to check whether the property is constant. Please review and let me know, thanks!
Hello! I’ve been busy with work. Have some time tomorrow, will get that done !
On Sat, 25 Jun 2022 at 9:15 PM Tim Fox @.***> wrote:
@.**** commented on this pull request.
In cluster/dragon/dragon.go https://github.com/cashapp/pranadb/pull/407#discussion_r906704941:
key = common.AppendUint32ToBufferBE(key, uint32(len(propKey)))key = append(key, propKey...)
- v, err := d.LocalGet(key)
- return d.LocalGet(key) +}
+func (d *Dragon) setConfigProperty(property string, v []byte) error {
Hi @mohsalsaleem https://github.com/mohsalsaleem I think we're almost there. If you can add a test, them we can get this merged!
— Reply to this email directly, view it on GitHub https://github.com/cashapp/pranadb/pull/407#discussion_r906704941, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSANGQEWVKQBKK4MGVFMV3VQ45DBANCNFSM5Y4MJBNA . You are receiving this because you were mentioned.Message ID: @.***>
-- Regards, V.H.Mohamed Saleem
No worries :) In the mean-time can you make sure you have signed the CLA, if you haven't done so already. https://docs.google.com/forms/d/e/1FAIpQLSeRVQ35-gq2vdSxD1kdh7CJwRdjmUA0EZ9gRXaWYoUeKPZEQQ/viewform?formkey=dDViT2xzUHAwRkI3X3k5Z0lQM091OGc6MQ&ndplr=1
Hello @purplefox, I'm a bit lost w.r.t writing test cases. Can you please help me with that? Currently, I can see how the shards are getting checked in the dragon_integration_test.go with TestGetAllShardIDs, where the shard length is checked against the configured shards count. Would it be okay if I did something similar to that where I stored the replication factor as a part of the Dragon struct and then getting and checking it against the configured value?
Yes, just add a test in dragon_integration_test.go which tests your new methods. You don't need to change the Dragon struct as the test already passes the replication factor in.
Hello @purplefox The method checkConstantProperty is not exported outside of the struct, hence a test could not be written in dragon_integration_test.go. I don't think it makes sense to export it, so is there another way with which we can possibly test that function?
Hi. Just checking in. Are you still working on this? If not, we can reassign to another team member.
Hi, apologies for being this late with the PR. I've added the test and made relevant changes. Please review, thanks!
The PR is failing lint checks. You can run lint manually from the pranadb project directory with:
bin/golangci-lint run