You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/07/07 07:45:40 UTC

[GitHub] [phoenix] kadirozde opened a new pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

kadirozde opened a new pull request #823:
URL: https://github.com/apache/phoenix/pull/823


   …amps set by clients


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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

[GitHub] [phoenix] gjacoby126 commented on pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

gjacoby126 commented on pull request #823:
URL: https://github.com/apache/phoenix/pull/823#issuecomment-655142442

@kadirozde - good point about the SCN. I think if this is configurable, there would need to be a validation check giving you an exception if you try to ask for both a client-set SCN and server side timestamps on mutations.

I like your suggestion about a given operation deterministically using either the client or server timestamp.

Seems to me the big question is how clients and servers decide which of those two to use. If it's the client's choice to either set or not set a timestamp, that still leaves the question of how the client chooses. Same if it's the server's choice -- is it a cluster setting, a table setting, or a hard-coded default? Dependent on mutable vs immutable? Lots of implications to work through for all possibilities, and I don't have a firm opinion yet.

For example, it would be good for a particular table to be able to override the default to say whether client or server-side timestamps should govern. I can also see it being useful for clients to request it as well as a connection property, as they do for SCN, but I worry a little about what happens if different clients make different choices...so I'm undecided.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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

[GitHub] [phoenix] gjacoby126 commented on pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

gjacoby126 commented on pull request #823:
URL: https://github.com/apache/phoenix/pull/823#issuecomment-654998520

@kadirozde - since HBase timestamps are ultimately derived from physical timestamps, rather than, say HLCs, we're already prone to weird effects from clock skew (such as different rs's having different clocks). This improves that in one aspect, since when the client-set timestamp "wins" it'll be consistent across region servers.

I'm curious about the implications of letting the server timestamp win when the server is behind the client. What if the client clock is slightly ahead of a server? Don't we go back to inconsistent timestamps then?

It seems like a preference for either the client or the server can cause skew-related problems.

When we honor the client timestamps, we get consistent timestamps across a logical operation no matter how many region servers it involves. However, we can get weird behavior where two clients with clock skew wrt each other can invert the order of operations.

When we honor the server timestamps, we can enforce an order between client operations regardless of the clients' clocks, but we can get inconsistent timestamps within each operation.

I can think of use cases I've worked with in the past that have a strong need for either consistent timestamps across a long-running operations and others with lots of small operations where order of operations across clients _really_ matters , so I'm thinking this needs to be configurable.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [phoenix] kadirozde commented on a change in pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

kadirozde commented on a change in pull request #823:
URL: https://github.com/apache/phoenix/pull/823#discussion_r450985622



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -817,7 +838,59 @@ private void testDeleteCount(boolean autoCommit, Integer limit) throws Exception
         }
 
     }
-    
+
+    private void testDeleteCountWithIndex(boolean autoCommit, boolean scn) throws Exception {
+        String tableName = generateUniqueName();
+        String tableDDL = "CREATE TABLE IF NOT EXISTS " + tableName + " (pk1 DECIMAL NOT NULL, v1 VARCHAR CONSTRAINT PK PRIMARY KEY (pk1))";
+        String indexName = generateUniqueName();
+        String indexDDL = "CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + "(v1)";
+        int numRecords = 1010;

Review comment:
       Each test takes about 5 to 10 seconds even with 1010 rows. I needed to see the same timestamp is used across multiple batches so I need to have several batches of rows. I have not injected an EnvironmentEdge and so no need to reset. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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

[GitHub] [phoenix] kadirozde commented on a change in pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

kadirozde commented on a change in pull request #823:
URL: https://github.com/apache/phoenix/pull/823#discussion_r450985622



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -817,7 +838,59 @@ private void testDeleteCount(boolean autoCommit, Integer limit) throws Exception
         }
 
     }
-    
+
+    private void testDeleteCountWithIndex(boolean autoCommit, boolean scn) throws Exception {
+        String tableName = generateUniqueName();
+        String tableDDL = "CREATE TABLE IF NOT EXISTS " + tableName + " (pk1 DECIMAL NOT NULL, v1 VARCHAR CONSTRAINT PK PRIMARY KEY (pk1))";
+        String indexName = generateUniqueName();
+        String indexDDL = "CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + "(v1)";
+        int numRecords = 1010;

Review comment:
       Each test takes about 5 to 10 seconds even with 1010 rows. I need to see the same timestamp is used across multiple batches so I need to have several batches of rows. I have not injected an EnvironmentEdge and so no need to reset. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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

[GitHub] [phoenix] gjacoby126 commented on a change in pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

gjacoby126 commented on a change in pull request #823:
URL: https://github.com/apache/phoenix/pull/823#discussion_r450943309



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -817,7 +838,59 @@ private void testDeleteCount(boolean autoCommit, Integer limit) throws Exception
         }
 
     }
-    
+
+    private void testDeleteCountWithIndex(boolean autoCommit, boolean scn) throws Exception {
+        String tableName = generateUniqueName();
+        String tableDDL = "CREATE TABLE IF NOT EXISTS " + tableName + " (pk1 DECIMAL NOT NULL, v1 VARCHAR CONSTRAINT PK PRIMARY KEY (pk1))";
+        String indexName = generateUniqueName();
+        String indexDDL = "CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + "(v1)";
+        int numRecords = 1010;

Review comment:
       If you do go that way, please remember to reset() the EnvironmentEdgeManager in a finally block

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -817,7 +838,59 @@ private void testDeleteCount(boolean autoCommit, Integer limit) throws Exception
         }
 
     }
-    
+
+    private void testDeleteCountWithIndex(boolean autoCommit, boolean scn) throws Exception {
+        String tableName = generateUniqueName();
+        String tableDDL = "CREATE TABLE IF NOT EXISTS " + tableName + " (pk1 DECIMAL NOT NULL, v1 VARCHAR CONSTRAINT PK PRIMARY KEY (pk1))";
+        String indexName = generateUniqueName();
+        String indexDDL = "CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + "(v1)";
+        int numRecords = 1010;

Review comment:
       do we need so many rows to test the timestamp behavior? How much time do these tests add to the test suite run? Seems like you could get the same effect in the scn case with 2 rows, using the edge manager to force a two millisecond time increment between the two upserts. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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

[GitHub] [phoenix] gjacoby126 edited a comment on pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

gjacoby126 edited a comment on pull request #823:
URL: https://github.com/apache/phoenix/pull/823#issuecomment-654998520

It seems like a preference for either the client or the server can cause skew-related problems.

When we honor the server timestamps, we can enforce an order between client operations regardless of the clients' clocks, but we can get inconsistent timestamps within each operation.

@gjacoby126 - Thank you for the feedback. I agree with on almost all of the points you made. I am not sure about having a configuration parameter about this due to the conflict with SCN queries. I think we need to honor the client timestamps and it will improve the current situation as you pointed out. Initially, I started with the following logic:
long now = getTimestamp(firstMutation);
if (now == HConstants.LATEST_TIMESTAMP) {
now = EnvironmentEdgeManager.currentTimeMillis()
}

Then I noticed that the client always sets the timestamp for mutations in my ITs. I changed the logic. I think as you said, we should either use the client or server timestamp. I will revert the logic, and so we will use the client timestamp when the client set it. What do you think?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [phoenix] gjacoby126 commented on a change in pull request #823: PHOENIX-5991 IndexRegionObserver should not overwrite mutation timest…

Posted by GitBox <gi...@apache.org>.

gjacoby126 commented on a change in pull request #823:
URL: https://github.com/apache/phoenix/pull/823#discussion_r451005061



##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java
##########
@@ -817,7 +838,59 @@ private void testDeleteCount(boolean autoCommit, Integer limit) throws Exception
         }
 
     }
-    
+
+    private void testDeleteCountWithIndex(boolean autoCommit, boolean scn) throws Exception {
+        String tableName = generateUniqueName();
+        String tableDDL = "CREATE TABLE IF NOT EXISTS " + tableName + " (pk1 DECIMAL NOT NULL, v1 VARCHAR CONSTRAINT PK PRIMARY KEY (pk1))";
+        String indexName = generateUniqueName();
+        String indexDDL = "CREATE INDEX IF NOT EXISTS " + indexName + " ON " + tableName + "(v1)";
+        int numRecords = 1010;

Review comment:
       OK, that's fine. Thanks!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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