-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a new transaction type for reading from secondary replicas #110
Conversation
protobuf/transaction.proto
Outdated
@@ -67,6 +67,7 @@ message Transaction { | |||
enum Type { | |||
READ = 0; | |||
WRITE = 1; | |||
READ_SECONDARY = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining cluster specific type here is not ideal, and I've created an issue for it: #113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not READ_REPLICA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I've answered it here on the other PR: https://github.com/graknlabs/client-java/pull/243/files#r563711191
But let's continue the discussion about the name here so that it's not scattered.
The reasoning to rename it to READ_SECONDARY
is because it allows you to read from secondary replicas, whereas READ
and WRITE
reads to the primary replica (ie., the term replica does not only refer only to the secondary one).
The alternative name would be READ_SECONDARY_REPLICA
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I can see the logic.
I'm not familiar enough with this domain to know what would/wouldn't be an appropriate name. READ_SECONDARY_REPLICA
sounds so damn long. I therefore propose that we go with READ_SECONDARY as you suggest and maybe revisit this after the first release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After chatting to Haikal we agreed to change READ_SECONDARY
from being a Transaction type to being an Option
named SECONDARY_REPLICA
or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated review to reflect recent discussion
964ef52
to
68cb20d
Compare
protobuf/options.proto
Outdated
@@ -44,4 +44,7 @@ message Options { | |||
oneof schema_lock_acquire_timeout_opt { | |||
int32 schema_lock_acquire_timeout_millis = 6; | |||
} | |||
oneof primary_replica_opt { | |||
bool primary_replica = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if primary_replica
is misleading because it could be switched off and the primary replica might still be used. What do you think of the following alternatives:
require_primary_replica
allow_secondary_replica
Or do you think we should stick with primary_replica
for conciseness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6d5a5e1.
What is the goal of this PR?
We've introduced a new Grakn-Cluster-specific transaction type, "read secondary". It is a read-only transaction that may read not only from primary but also from secondary replicas.
What are the changes implemented in this PR?
allow_secondary_replica