[GCS]Support gcs client job&actor&node table subscribe idempotent#9424
[GCS]Support gcs client job&actor&node table subscribe idempotent#9424ffbin wants to merge 18 commits intoray-project:masterfrom
Conversation
|
Can one of the admins verify this patch? |
|
Test FAILed. |
|
Test PASSed. |
|
What's the relationship to this PR btw? #9153 |
1a27d3f to
9ad2794
Compare
|
Test PASSed. |
|
Test PASSed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
stephanie-wang
left a comment
There was a problem hiding this comment.
Actor table idempotence is already implemented in #9449 and the node table should already be idempotent. I'm not familiar with what the job table does, but it looks like it's just marking jobs as dead, which is idempotent too.
So do we actually need this PR?
New changes in notification handlers could easily introduce bugs without being noticed. In GCS client to ensure message processing idempotent, we can develop more simple and efficient. |
But there is no point in making message processing in the GCS idempotent if the specific table logic is already idempotent. The problem has already been fixed, so this is just adding unnecessary complexity. I'm not worried about changes in the notification handlers because these can be easily unit tested so we would definitely notice any new bugs. I suggest we close this PR. |
|
Closing this PR for now since I don't believe the PR is necessary and it's now been 1 month. Feel free to reopen if there are more specific changes that need to be made. |
Why are these changes needed?
Design document:
https://docs.google.com/document/d/1Cuqxlw53abEZPVYVF-pUWpbnwrFEgVTel_sQGwt8DmU/edit#heading=h.s3ogmk8m7308
This PR implements GCS client subscribe idempotent.
Node table and object location table are special in that they contain several records, so they are sorted by timestamp in GCS, I will implement it in the next pr.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.