Re: Review Request 71589: Create read-only transactions

2019-10-31 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 31, 2019, 3:21 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 355c4f5374 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 b5f22092a9 


Diff: https://reviews.apache.org/r/71589/diff/7/

Changes: https://reviews.apache.org/r/71589/diff/6-7/


Testing
---

Unit + manual test


File Attachments


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/b14eedb4-a2f1-4f77-9676-c258b6804b98__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/9096f402-3d2e-4cd2-9f85-df1dfeb25863__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/28/a001316c-bcf4-43a2-83fa-7d49183b2a7f__HIVE-21114.8.patch
HIVE-21114.10.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/31/93854078-7b63-46ec-95a0-62ab783ee54c__HIVE-21114.10.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-31 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 31, 2019, 3:20 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 355c4f5374 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 b5f22092a9 


Diff: https://reviews.apache.org/r/71589/diff/6/


Testing
---

Unit + manual test


File Attachments (updated)


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/b14eedb4-a2f1-4f77-9676-c258b6804b98__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/9096f402-3d2e-4cd2-9f85-df1dfeb25863__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/28/a001316c-bcf4-43a2-83fa-7d49183b2a7f__HIVE-21114.8.patch
HIVE-21114.10.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/31/93854078-7b63-46ec-95a0-62ab783ee54c__HIVE-21114.10.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-30 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 30, 2019, 12:23 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 355c4f5374 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 b5f22092a9 


Diff: https://reviews.apache.org/r/71589/diff/6/

Changes: https://reviews.apache.org/r/71589/diff/5-6/


Testing
---

Unit + manual test


File Attachments


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/b14eedb4-a2f1-4f77-9676-c258b6804b98__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/9096f402-3d2e-4cd2-9f85-df1dfeb25863__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/28/a001316c-bcf4-43a2-83fa-7d49183b2a7f__HIVE-21114.8.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-28 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 28, 2019, 1:40 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 504e6b12a1 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 46fc3ed1e0 


Diff: https://reviews.apache.org/r/71589/diff/5/


Testing
---

Unit + manual test


File Attachments (updated)


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/b14eedb4-a2f1-4f77-9676-c258b6804b98__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/9096f402-3d2e-4cd2-9f85-df1dfeb25863__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/28/a001316c-bcf4-43a2-83fa-7d49183b2a7f__HIVE-21114.8.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-22 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 22, 2019, 1:24 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 504e6b12a1 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 46fc3ed1e0 


Diff: https://reviews.apache.org/r/71589/diff/5/

Changes: https://reviews.apache.org/r/71589/diff/4-5/


Testing
---

Unit + manual test


File Attachments (updated)


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/b14eedb4-a2f1-4f77-9676-c258b6804b98__HIVE-21114.8.patch
HIVE-21114.8.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/22/9096f402-3d2e-4cd2-9f85-df1dfeb25863__HIVE-21114.8.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-17 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 17, 2019, 12:37 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 


Diff: https://reviews.apache.org/r/71589/diff/4/

Changes: https://reviews.apache.org/r/71589/diff/3-4/


Testing
---

Unit + manual test


File Attachments (updated)


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
HIVE-21114.5.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/17/80cbb092-97d6-48d2-b603-24213141cb5e__HIVE-21114.5.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-17 Thread Peter Vary via Review Board 


> On okt. 17, 2019, 11:10 de, Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 992 (original), 1003 (patched)
> > 
> >
> > Why is this a List of Pairs, why not just a Map? Is the order important?
> 
> Denys Kuzmenko wrote:
> Have no idea :) Originally I refactored it to the Map and than reverted 
> back to List of Pairs as it required some more refactoring in Driver. If you 
> are raising same concern, probably makes sence to change this.

Here or in an follow-up might worth to clean-up -> This seems very strange for 
me too


- Peter


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218258
---


On okt. 17, 2019, 8:41 de, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 17, 2019, 8:41 de)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/3/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> File Attachments
> 
> 
> HIVE-21114.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-17 Thread Denys Kuzmenko via Review Board 


> On Oct. 17, 2019, 11:10 a.m., Peter Vary wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Line 992 (original), 1003 (patched)
> > 
> >
> > Why is this a List of Pairs, why not just a Map? Is the order important?

Have no idea :) Originally I refactored it to the Map and than reverted back to 
List of Pairs as it required some more refactoring in Driver. If you are 
raising same concern, probably makes sence to change this.


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218258
---


On Oct. 17, 2019, 8:41 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated Oct. 17, 2019, 8:41 a.m.)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/3/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> File Attachments
> 
> 
> HIVE-21114.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-17 Thread Peter Vary via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218258
---



nits and a single question


ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 956 (patched)


Remove this " + " please :)



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 985 (original), 989 (patched)


nit: I hate your naming :) tableCol is table column for me :D



ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 992 (original), 1003 (patched)


Why is this a List of Pairs, why not just a Map? Is the order important?


- Peter Vary


On okt. 17, 2019, 8:41 de, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 17, 2019, 8:41 de)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/3/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> File Attachments
> 
> 
> HIVE-21114.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-17 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 17, 2019, 8:41 a.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 91910d1c0c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 


Diff: https://reviews.apache.org/r/71589/diff/3/

Changes: https://reviews.apache.org/r/71589/diff/2-3/


Testing
---

Unit + manual test


File Attachments


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-14 Thread Peter Vary via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218201
---




ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
Lines 48 (patched)


Ohh, and another important As -> AS :D


- Peter Vary


On okt. 10, 2019, 4:09 du, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 10, 2019, 4:09 du)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/2/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> File Attachments
> 
> 
> HIVE-21114.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-14 Thread Peter Vary via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218200
---



+1 pending tests.
And some nits, just to be constructive :D :D :D


ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Line 355 (original), 356 (patched)


Unnecessary change



ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
Lines 34 (patched)


Maybe: WITH a AS (SELECT...
But this is really the nit of nits :D :D :D



ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
Lines 41 (patched)


Maybe: WITH a AS (SELECT...
But this is really the nit of nits :D :D :D


- Peter Vary


On okt. 10, 2019, 4:09 du, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 10, 2019, 4:09 du)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/2/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> File Attachments
> 
> 
> HIVE-21114.1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 10, 2019, 4:09 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 


Diff: https://reviews.apache.org/r/71589/diff/2/

Changes: https://reviews.apache.org/r/71589/diff/1-2/


Testing
---

Unit + manual test


File Attachments


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

(Updated Oct. 10, 2019, 4:09 p.m.)


Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 


Diff: https://reviews.apache.org/r/71589/diff/1/


Testing
---

Unit + manual test


File Attachments (updated)


HIVE-21114.1.patch
  
https://reviews.apache.org/media/uploaded/files/2019/10/10/0929ed4a-17be-4098-8c61-0819a30613fd__HIVE-21114.1.patch


Thanks,

Denys Kuzmenko

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Denys Kuzmenko via Review Board 


> On Oct. 10, 2019, 7:46 a.m., Peter Vary wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
> > Lines 47 (patched)
> > 
> >
> > What about CREATE TABLE AS SELECT * FROM...?
> > We still might miss some cases.
> > 
> > I would create an assertion on generating writeId for read only 
> > transactions (this would be usefull anyway), and use the ptest to run on 
> > all of the test cases to see if this assertion breaks anything.
> > 
> > What do you think?
> 
> Denys Kuzmenko wrote:
> Hi Peter, yes, I was thinking about something similar, however most 
> propably it would be one time check (won't be commited). 
> Somewhere in Driver.compile method, after SemanticAnalyzer add assert if 
> transaction marked as ReadOnly doesn't have assosiated write ids. 
> This way we could make sure we do not misclasify some of the existing 
> queries. Does this makes sence?
> 
> Peter Vary wrote:
> I would vote for the check to be committed for several reasons:
> - We might cause strange/flaky errors if we assume that a transacion is 
> RO, but in reality it writes something. Easier to catch this if we fail fast.
> - When introducing new commands, it would be easy to forget to update 
> this check, but if the assertion is there we will catch them compile time - 
> again fail fast
> 
> Just my 2 cents

Agreed! Thank you, Peter!


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218174
---


On Oct. 8, 2019, 2:27 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated Oct. 8, 2019, 2:27 p.m.)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/1/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Peter Vary via Review Board 


> On okt. 10, 2019, 7:46 de, Peter Vary wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
> > Lines 47 (patched)
> > 
> >
> > What about CREATE TABLE AS SELECT * FROM...?
> > We still might miss some cases.
> > 
> > I would create an assertion on generating writeId for read only 
> > transactions (this would be usefull anyway), and use the ptest to run on 
> > all of the test cases to see if this assertion breaks anything.
> > 
> > What do you think?
> 
> Denys Kuzmenko wrote:
> Hi Peter, yes, I was thinking about something similar, however most 
> propably it would be one time check (won't be commited). 
> Somewhere in Driver.compile method, after SemanticAnalyzer add assert if 
> transaction marked as ReadOnly doesn't have assosiated write ids. 
> This way we could make sure we do not misclasify some of the existing 
> queries. Does this makes sence?

I would vote for the check to be committed for several reasons:
- We might cause strange/flaky errors if we assume that a transacion is RO, but 
in reality it writes something. Easier to catch this if we fail fast.
- When introducing new commands, it would be easy to forget to update this 
check, but if the assertion is there we will catch them compile time - again 
fail fast

Just my 2 cents


- Peter


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218174
---


On okt. 8, 2019, 2:27 du, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 8, 2019, 2:27 du)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/1/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Denys Kuzmenko via Review Board 


> On Oct. 10, 2019, 7:46 a.m., Peter Vary wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
> > Lines 47 (patched)
> > 
> >
> > What about CREATE TABLE AS SELECT * FROM...?
> > We still might miss some cases.
> > 
> > I would create an assertion on generating writeId for read only 
> > transactions (this would be usefull anyway), and use the ptest to run on 
> > all of the test cases to see if this assertion breaks anything.
> > 
> > What do you think?

Hi Peter, yes, I was thinking about something similar, however most propably it 
would be one time check (won't be commited). 
Somewhere in Driver.compile method, after SemanticAnalyzer add assert if 
transaction marked as ReadOnly doesn't have assosiated write ids. 
This way we could make sure we do not misclasify some of the existing queries. 
Does this makes sence?


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218174
---


On Oct. 8, 2019, 2:27 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated Oct. 8, 2019, 2:27 p.m.)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/1/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 71589: Create read-only transactions

2019-10-10 Thread Peter Vary via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/#review218174
---



One more question to find a way to identify write queries.
Otherwise as discussed, I do not see a better way to check the type fo the 
transaction :(


ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java
Lines 47 (patched)


What about CREATE TABLE AS SELECT * FROM...?
We still might miss some cases.

I would create an assertion on generating writeId for read only 
transactions (this would be usefull anyway), and use the ptest to run on all of 
the test cases to see if this assertion breaks anything.

What do you think?


- Peter Vary


On okt. 8, 2019, 2:27 du, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71589/
> ---
> 
> (Updated okt. 8, 2019, 2:27 du)
> 
> 
> Review request for hive, Laszlo Pinter and Peter Vary.
> 
> 
> Bugs: HIVE-21114
> https://issues.apache.org/jira/browse/HIVE-21114
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> With HIVE-21036 we have a way to indicate that a txn is read only.
> We should (at least in auto-commit mode) determine if the single stmt is a 
> read and mark the txn accordingly.
> Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks 
> in write_set etc.
> 
> TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
> TXN_OPEN) so it can read the txn type in the same SQL stmt.
> 
> HiveOperation only has QUERY, which includes Insert and Select, so this 
> requires figuring out how to determine if a query is a SELECT. By the time 
> Driver.openTransaction(); is called, we have already parsed the query so 
> there should be a way to know if the statement only reads.
> 
> For multi-stmt txns (once these are supported) we should allow user to 
> indicate that a txn is read-only and then not allow any statements that can 
> make modifications in this txn. This should be a different jira.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> ac813c8288 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 1c53426966 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
> cc86afedbf 
>   ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71589/diff/1/
> 
> 
> Testing
> ---
> 
> Unit + manual test
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Review Request 71589: Create read-only transactions

2019-10-08 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71589/
---

Review request for hive, Laszlo Pinter and Peter Vary.


Bugs: HIVE-21114
https://issues.apache.org/jira/browse/HIVE-21114


Repository: hive-git


Description
---

With HIVE-21036 we have a way to indicate that a txn is read only.
We should (at least in auto-commit mode) determine if the single stmt is a read 
and mark the txn accordingly.
Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks in 
write_set etc.

TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, 
TXN_OPEN) so it can read the txn type in the same SQL stmt.

HiveOperation only has QUERY, which includes Insert and Select, so this 
requires figuring out how to determine if a query is a SELECT. By the time 
Driver.openTransaction(); is called, we have already parsed the query so there 
should be a way to know if the statement only reads.

For multi-stmt txns (once these are supported) we should allow user to indicate 
that a txn is read-only and then not allow any statements that can make 
modifications in this txn. This should be a different jira.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java ac813c8288 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 1c53426966 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 
cc86afedbf 
  ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java PRE-CREATION 


Diff: https://reviews.apache.org/r/71589/diff/1/


Testing
---

Unit + manual test


Thanks,

Denys Kuzmenko