[GitHub] sijie commented on a change in pull request #898: Bookies should prioritize recovery reads/writes

[GitBox <gi...@apache.org>]

sijie commented on a change in pull request #898: Bookies should prioritize recovery reads/writes
URL: https://github.com/apache/bookkeeper/pull/898#discussion_r169797714
 
 

 ##########
 File path: bookkeeper-proto/src/main/proto/BookkeeperProtocol.proto
 ##########
 @@ -90,8 +90,9 @@ message ReadRequest {
     enum Flag {
         FENCE_LEDGER = 1;
         ENTRY_PIGGYBACK = 2;
+        HIGH_PRIORITY = 3;
     }
-    optional Flag flag = 100;
+    repeated Flag flag = 100;
 
 Review comment:
   hmm I think this changes too much for a portback. i know repeated field is compatible with optional. However I am not sure how it would impact other existing code logic. I don't feel comfortable with change, especially this is coupled with a portback change.
   
   This definitely requires more thoughts and testing. I would suggest not changing it to repeated at this moment. since v2 and v3 are different protocols, we don't necessarily need to follow what v2 is doing for v3. If it is a 'priority' field, we should just separate it from a flag. keep flag as it is and adding a field called `priority` in the request header. so in this way, you don't need to change the Flag field from optional to repeated. And the priority field can then be used later to further optimize io scheduler if we want to do.
   
   There is no BC concern in v3 protocol, since pulsar is using v2 protocol only. The BC concern between apache version and yahoo version only remains at v2 protocol.
   
   If this approach doesn't make sense to you, I would suggest decoupling changing v3 protocol from this PR, get the change in v2 first to complete this merge. since this is required by pulsar. Defer changing v3 protocol in a different PR. I would prefer not take any risks on this, because except pulsar, most of the people is using v3 protocol. And this feature is used by pulsar in v2 protocol only.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services