Skip to content
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

Issue/JCES-1896 Adding isWrite information to API #49

Conversation

afaruga-atlassian
Copy link
Contributor

JCES-1896 draft - I'm trying to find the ultimate answer for how to judge whether sql is read/write and where is best place (is there possibility to increase coverage by more readQueries moved to Replica). How SQL operation is treated in db-replica is based on statementType + sql + consistency. Is there a way to arbitrarily judge whether SQL is read/write?

@afaruga-atlassian
Copy link
Contributor Author

Screenshot 2021-03-02 at 10 13 12
Github is cool. I didn't know about this feature.
Thanks @dagguh !

@wyrzyk
Copy link
Contributor

wyrzyk commented Mar 2, 2021

Is there a way to arbitrarily judge whether SQL is read/write?

Can we start from the context? Why do we need to know if it's a read/write operation?

Some operations may be read-only but will never reach replica. It also depends on the context.

How SQL operation is treated in db-replica is based on statementType + sql + consistency

  • context/status, transaction isolation level (I'm not 100% sure if it covers all the ingredients)

@afaruga-atlassian
Copy link
Contributor Author

Can we start from the context? Why do we need to know if it's a read/write operation?

Context is, that for resource owner(REST endpoint/ amq/other) we want to let them monitor their endpoints whether actually, they are readonly, and later help them maintain that readability, to give them API that they can implement a way how to react if somebody broke their resource and make it none-readonly again, thats the idea.
Thanks for this resource owner will hope that its traffic was moved to RR.

So the question is?
Whether Resource owner is interesting on fixed information based on sql whether its resource is ReadOnly, or whether its resource its treated by db-replica as treat only, what is more complex. 😕

Some operations may be read-only but will never reach replica. It also depends on the context.

What's the reason? Isn't permanent, or temporary(early stages of db-replica evolution), mixed?

I can imagine that explaining the full context might be tricky and there is still some space to discover and learn about it.

@dagguh
Copy link
Contributor

dagguh commented Mar 2, 2021

Can we start from the context? Why do we need to know if it's a read/write operation?

I described the use case in #50

@dagguh dagguh linked an issue Mar 2, 2021 that may be closed by this pull request
@wyrzyk
Copy link
Contributor

wyrzyk commented Mar 2, 2021

Context is, that for resource owner(REST endpoint/ amq/other) we want to let them monitor their endpoints whether actually, they are readonly, and later help them maintain that readability, to give them API that they can implement a way how to react if somebody broke their resource and make it none-readonly again, thats the idea.

It seems the resource owner wants to know if queries are landing on replica or main and not if the queries are read-only or not. The API to do that already exist, if DatabaseCall uses only the above reasons, then it will work with replica-only:

  public static final Reason REPLICA_INCONSISTENT = new Reason("REPLICA_INCONSISTENT", true);
    public static final Reason READ_OPERATION = new Reason("READ_OPERATION", false);
    public static final Reason MAIN_CONNECTION_REUSE = new Reason( "MAIN_CONNECTION_REUSE",    true );
    public static final Reason RO_API_CALL = new Reason("RO_API_CALL", false);

REPLICA_INCONSISTENT -> it's outside of control for a single endpoint.
MAIN_CONNECTION_REUSE -> may be caused by REPLICA_INCONSISTENT

@afaruga-atlassian
Copy link
Contributor Author

afaruga-atlassian commented Mar 2, 2021

Context is, that for resource owner(REST endpoint/ amq/other) we want to let them monitor their endpoints whether actually, they are readonly, and later help them maintain that readability, to give them API that they can implement a way how to react if somebody broke their resource and make it none-readonly again, thats the idea.

In my opinion Resource owner would like to know what it can do to make resource RR compatible
Temporary issues like an inconsistent replica, and final outcome (whether it was run on main) might be useful to

Full data (the data might change as db-replica implementation will growth and make decision more complicated) :
IsSQLWriteOpeartaion, wasItRunOnMain, wasReplica consistent for this query.

It seems the resource owner wants to know if queries are landing on replica or main and not if the queries are read-only or not. The API to do that already exist, if DatabaseCall uses only the above reasons, then it will work with replica-only:

  public static final Reason REPLICA_INCONSISTENT = new Reason("REPLICA_INCONSISTENT", true);
    public static final Reason READ_OPERATION = new Reason("READ_OPERATION", false);
    public static final Reason MAIN_CONNECTION_REUSE = new Reason( "MAIN_CONNECTION_REUSE",    true );
    public static final Reason RO_API_CALL = new Reason("RO_API_CALL", false);

REPLICA_INCONSISTENT -> it's outside of control for a single endpoint.
MAIN_CONNECTION_REUSE -> may be caused by REPLICA_INCONSISTENT

So for REPLICA_INCONSISTENT and MAIN_CONNECTION_REUSE what information Resource owner should receive?
was it Write? Thus UNKNOWN.

@wyrzyk
Copy link
Contributor

wyrzyk commented Mar 2, 2021

In my opinion Resource owner would like to know what it can do to make resource RR compatible

The resource owner will get the SQL. The user can also capture a stack-trace. We can't provide more information than that. All our internal classifications will always be less accurate than this (SQL query and context are inputs).

And then there are two cases:

  1. It's a write operation -> change the code in the application to avoid it or don't assume the endpoint is read-only.
  2. It's a write operation -> create an issue (or PR), there's a bug in the library.

So for REPLICA_INCONSISTENT and MAIN_CONNECTION_REUSE what information Resource owner should receive?

REPLICA_INCONSISTENT -> Consistency of the replica reduces the effectiveness of the library. The user can experiment with better consistency implementations or with losing some requirements.

MAIN_CONNECTION_REUSE -> it will always come with a reason. It means it was already decided, and the current decision doesn't matter. User can dig into the cause getCause.

wasItRunOnMain, wasReplica

It never was. DatabaseCall implementation controls the database call.

@afaruga-atlassian
Copy link
Contributor Author

I've reused stateListner to learn more about states changed behind the scene. It was much more productive than debugging.
Double responsibility of RouteDecisionBuilder to choose/mark statement(read/write) and connectionState adding coupling and make it a bit more complex. We might think about splitting it in the future. 🤷

@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch from e5dfced to c2cb4cc Compare March 4, 2021 15:47
@afaruga-atlassian afaruga-atlassian marked this pull request as ready for review March 8, 2021 07:01
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch 4 times, most recently from 51832fc to 631b6bb Compare March 8, 2021 07:55
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch from 1a7bfec to b793b34 Compare March 8, 2021 08:48
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch 2 times, most recently from cea5801 to f807cfc Compare March 9, 2021 07:15
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch 2 times, most recently from 357e9d7 to e4bb2fc Compare March 9, 2021 13:48
…on on compile lvl to Reason and make it main source of truth for isWriteOperation. Introduced SqlQuery with OO goodness. Only (RW_API_CALL, WRITE_OPERATION) are Write reasons, rest is Read reasons, but might be run on MAIN due to connection state and other condtions
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch from 5f2ee15 to bd0a683 Compare March 9, 2021 16:26
dagguh
dagguh previously approved these changes Mar 9, 2021
…n.isWrite(), introducing SqlQuery to improve OO impl
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch 2 times, most recently from 64c2001 to e3db3db Compare March 9, 2021 17:13
@afaruga-atlassian afaruga-atlassian force-pushed the issue/JCES-1896-isWrite-api-sql-draft branch from e3db3db to 147f99b Compare March 9, 2021 23:31
@afaruga-atlassian
Copy link
Contributor Author

@wyrzyk because you requested change requested I need your response to move forward with PR

@afaruga-atlassian afaruga-atlassian merged commit 09757be into atlassian-labs:master Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help me ensure I'm a read-only consumer
4 participants