HDDS-5954. EC: Review the TODOs in GRPC Xceiver client and fix them. #2979
Conversation
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
Thanks @fapifta for working on this. I have just dropped few comments. Please take a look. Thanks
| are sent in via multiple message streams on the same channel. | ||
|
|
||
| ### When client code uses synced calls | ||
| There is no additional responsibilities on the client code about single |
There was a problem hiding this comment.
There is -> There are
| ### Why we just do not engage in this | ||
| This whole question came up as part of Erasure Coded storage development, and | ||
| in the EC write path, we have to synchronize at multiple points of a write. | ||
| Once an EC data block group is written, we need to ensure all the data and |
There was a problem hiding this comment.
it should be at stripe level, but not for whole block group. We do put block on every stripe write finished.
| * replicate to any other nodes, usage of this client implementation to | ||
| * write replicated data is ill-advised. | ||
| * | ||
| * When using the sendCommandAsync method, the client code need to ensure |
There was a problem hiding this comment.
Isn't it a good idea to add java doc at API level(sendCommandAsync) instead of at class level itself?
There was a problem hiding this comment.
removed this clause, as we need to leave the original client this way, and we should specialize the implementation for EC only, as Standalone client is still not deprecated for writes, and we should not change its semantics directly
| thread where it has arrived, but if we move that to a background thread, and | ||
| send the response when it is ready, we loose the ordering guarantee of gRPC... | ||
|
|
||
| So, all in all, the solution that hurst us the least is to ensure that, |
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| --> | ||
|
|
There was a problem hiding this comment.
I am not very sure, what is the right place for this kind of documents?
README generally we provide for users to let them know how to use it, but this is more like internal dev doc for understanding on usage/how it works?
There was a problem hiding this comment.
This is generally a documentation for the user's of this class to know how the internals work, so I would say technically it is a user doc, but I would not publish it in the Ozone doc, as it is not for Ozone users... I am not sure even what would be the right place, this was my best bet so far, but I am open to ideas if there is a better place for this doc.
| development we already selected the standalone client for writes. That effort | ||
| initiated the analysis of the client side, and this writeup. | ||
| The plan is to continue to use the gRPC client only for reads, and for EC writes | ||
| in the future, if that changes, then this document might as well need an update. |
There was a problem hiding this comment.
It is very easy to miss to update this document if we change our mind to use some other ways that GrpcClient for EC writes. If we expect this doc to be updated, we may need to provide back reference in relevant java file.
There was a problem hiding this comment.
I prefer this kind of detailed docs can be at javadocs also, but this looks to be having too many details.
There was a problem hiding this comment.
In #3012 I have referred the file from the class' API doc, and also I removed the sentence starting as "The plan is..." this plan is documented in the JIRA tasks, and will be visible as the code evolves, when changes to be made, hopefully one does not forget to update this doc, if it is necessary.
|
Hi @umamaheswararao, thank you for the reviews, I will incorporate these comments into the version I will post under the new JIRA tickets, as I separated out 4 different things that we need to do in order to get through this task.
With that I am closing this PR, and will post the new ones later today, as I am moving out the different pieces to different branches. |
2 participants
What changes were proposed in this pull request?
Added an extensive documentation about the gRPC client code that we have for reads, and also we are using for EC writes already in the EC development branch.
This documentation justifies as well the changes proposed in the XCeiverClientGrpc class.
The changes are:
Some of the wording in the documentation contains information about the current EC development status, these to be updated once we have merged the EC code to the master branch. For now they are there to help understand some of the internals that are relevant for the write requests, and for the decisions made about how to resolve the TODO items that are removed from the XCeiverClientGrpc class as they are considered to be resolved, or abandoning of them is justified in the documentation.
As the changes does not have any visible effect on how the master branch works, I am proposing to merge this change to the master branch directly, to ease the EC branch merge work later.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5954
How was this patch tested?
Same JUnit tests should cover the behaviour in the master branch as before, as changes are not affecting the logic used in the current codebase.
In EC branch, I have tried to run our current tests, and I have not seen issues to appear, the only logical change should have an effect in the EC write path, and is covered with EC tests and backed up by external synchronization points in the EC code.