Implement the KvBackend trait for MetaPeerClient

[WenyXu]

          Nonblocking: I think it seems necessary to implement the KvBackend trait for MetaPeerClient, perhaps in the next PR.

Originally posted by @fengjiachun in #3032 (comment)

[lyang24]

Hi I am interested in this issue may i work on this one?

[WenyXu]

@lyang24 Thanks, fave fun!🤩

[lyang24]

Hi @WenyXu , I have a draft on this issue and i have a few questions.

1. seems like MetaPeerClient only offer read-only methods should we leave all the write method unimplemented?
2. The original get method on MetaPeerClient is very similar to the default get method on KvBackend trait except the debug assert on length of the kv returned. should we just use the default impl provided by the trait?
3. The draft got tagged 'need doc update' can you please help me understand what needs to be updated? (is there a change log or something)?

Thank you in advance and no rush on these questions :)

[WenyXu]

Hi @WenyXu , I have a draft on this issue and i have a few questions.

1. seems like MetaPeerClient only offer read-only methods should we leave all the write method unimplemented?
2. The original get method on MetaPeerClient is very similar to the default get method on KvBackend trait except the debug assert on length of the kv returned. should we just use the default impl provided by the trait?
3. The draft got tagged 'need doc update' can you please help me understand what needs to be updated? (is there a change log or something)?

Thank you in advance and no rush on these questions :)

Hi, @lyang24. Thank you for your concern.

• For question 1, It seems the ClusterClient implementation only allows us to implement immutable methods; maybe we should throw an error if users try to invoke any mutable methods of the MetaPeerClient. cc @fengjiachun @fengys1996
• And about question 2, Yes, let's use the default implement and remove the debug asserts.
• For the last question, I added a mark in your PR's checkbox(the This PR does not require documentation updates.); the bot will remove the need doc update label.

Feel free to let me know if you have any other questions😉

[lyang24]

Thank you, I have addressed the issues and made pr ready for review. Seem like the method of MetaPeerClient is not easy to test since it involves grpc calls, let me know is there any test i can add here.

[tisonkun]

Hi @WenyXu @lyang24! Shall we close this issue as done as #3076 merged, or it's open for adding some tests around the patch?

[WenyXu]

Hi @WenyXu @lyang24! Shall we close this issue as done as #3076 merged, or it's open for adding some tests around the patch?

Let’s close it