You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Daniel Dai <da...@gmail.com> on 2016/12/16 23:14:40 UTC
Review Request 54826: HIVE-15448: ChangeManager for replication
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/
-----------------------------------------------------------
Review request for hive and Thejas Nair.
Repository: hive-git
Description
-------
See HIVE-15448
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9064e49
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java f7b2ed7
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
Diff: https://reviews.apache.org/r/54826/diff/
Testing
-------
Thanks,
Daniel Dai
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
> On Dec. 27, 2016, 6:26 a.m., Thejas Nair wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java, line 55
> > <https://reviews.apache.org/r/54826/diff/1-2/?file=1588285#file1588285line55>
> >
> > seems better as local to the reycle method, as its not used outside.
I mean to cache user/group as it should not be changed. I don't want to calculate them everytime, and taking risk current user being changed unexpected.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review160135
-----------------------------------------------------------
On Dec. 27, 2016, 6:55 a.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 27, 2016, 6:55 a.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
> On Dec. 27, 2016, 6:26 a.m., Thejas Nair wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java, line 55
> > <https://reviews.apache.org/r/54826/diff/1-2/?file=1588285#file1588285line55>
> >
> > seems better as local to the reycle method, as its not used outside.
>
> Daniel Dai wrote:
> I mean to cache user/group as it should not be changed. I don't want to calculate them everytime, and taking risk current user being changed unexpected.
Put in the construct
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review160135
-----------------------------------------------------------
On Dec. 27, 2016, 6:55 a.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 27, 2016, 6:55 a.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review160135
-----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 55)
<https://reviews.apache.org/r/54826/#comment231174>
seems better as local to the reycle method, as its not used outside.
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 57)
<https://reviews.apache.org/r/54826/#comment231177>
trailing whitespaces
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 150)
<https://reviews.apache.org/r/54826/#comment231173>
how about doing this as part of ReplChangeManager constructor ? That will reduce the number of times this needs to be called.
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 165)
<https://reviews.apache.org/r/54826/#comment231175>
trailing whitespaces in this change. Shows up in red in the diff.
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 172)
<https://reviews.apache.org/r/54826/#comment231176>
trailing whitespaces
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 232)
<https://reviews.apache.org/r/54826/#comment231167>
check for DEBUG enabled, but logging is at INFO level
- Thejas Nair
On Dec. 23, 2016, 8:29 a.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2016, 8:29 a.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review160177
-----------------------------------------------------------
Ship it!
Ship It!
- Thejas Nair
On Dec. 27, 2016, 4:44 p.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 27, 2016, 4:44 p.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/
-----------------------------------------------------------
(Updated Dec. 27, 2016, 4:44 p.m.)
Review request for hive and Thejas Nair.
Repository: hive-git
Description
-------
See HIVE-15448
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
Diff: https://reviews.apache.org/r/54826/diff/
Testing
-------
Thanks,
Daniel Dai
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/
-----------------------------------------------------------
(Updated Dec. 27, 2016, 6:55 a.m.)
Review request for hive and Thejas Nair.
Repository: hive-git
Description
-------
See HIVE-15448
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
Diff: https://reviews.apache.org/r/54826/diff/
Testing
-------
Thanks,
Daniel Dai
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/
-----------------------------------------------------------
(Updated Dec. 23, 2016, 8:29 a.m.)
Review request for hive and Thejas Nair.
Repository: hive-git
Description
-------
See HIVE-15448
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b4e89b0
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2892da3
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
Diff: https://reviews.apache.org/r/54826/diff/
Testing
-------
Thanks,
Daniel Dai
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Daniel Dai <da...@gmail.com>.
> On Dec. 20, 2016, 8:56 p.m., Thejas Nair wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java, line 175
> > <https://reviews.apache.org/r/54826/diff/1/?file=1588285#file1588285line175>
> >
> > is there any generic way to find this from FileSystem api ? these configs are specific to HDFS.
> > But i guess we don't need this if we go with storing only the signature (see following comment)
I checked HDFS code, doesn't seems another way to do that
> On Dec. 20, 2016, 8:56 p.m., Thejas Nair wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java, line 56
> > <https://reviews.apache.org/r/54826/diff/1/?file=1588283#file1588283line56>
> >
> > with recent changes by Vaibhav to ProxyFileSystem, local files would also have checksums. Can we switch to local files, so that test speed is better (local fs might be more reliable than minidfs, as it has less parts to it!)
setXAttr will need HDFS
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review159520
-----------------------------------------------------------
On Dec. 16, 2016, 11:14 p.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2016, 11:14 p.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9064e49
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java f7b2ed7
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>
Re: Review Request 54826: HIVE-15448: ChangeManager for replication
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54826/#review159520
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 440)
<https://reviews.apache.org/r/54826/#comment230534>
lets keep this disabled for now as this work is in early stages
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 442)
<https://reviews.apache.org/r/54826/#comment230535>
any HCFS should work for this (Hadoop compatible file system) (at least eventually). I think we can remove the HDFS reference.
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 443)
<https://reviews.apache.org/r/54826/#comment230536>
should we use hours instead as default unit ? it seems most people would think of this parameter in hours/days.
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java (line 56)
<https://reviews.apache.org/r/54826/#comment230765>
with recent changes by Vaibhav to ProxyFileSystem, local files would also have checksums. Can we switch to local files, so that test speed is better (local fs might be more reliable than minidfs, as it has less parts to it!)
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java (line 59)
<https://reviews.apache.org/r/54826/#comment230766>
why is this needed ?
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java (line 124)
<https://reviews.apache.org/r/54826/#comment230768>
how about creating a method for this and calling -Partition part1 = createAndPartition("20160101")
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java (line 241)
<https://reviews.apache.org/r/54826/#comment230772>
can you add a comment like -
// verify cm.recycle(Path) api moves file to cmroot dir
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java (line 247)
<https://reviews.apache.org/r/54826/#comment230773>
// verify cm.recycle(db, table) api moves file to cmroot dir
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 151)
<https://reviews.apache.org/r/54826/#comment230782>
should we call it getCksumString() to make it more easy to understand what is expected ?
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 175)
<https://reviews.apache.org/r/54826/#comment230776>
is there any generic way to find this from FileSystem api ? these configs are specific to HDFS.
But i guess we don't need this if we go with storing only the signature (see following comment)
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 182)
<https://reviews.apache.org/r/54826/#comment230780>
as discussed offline, looks like we don't want to rely on the directory structure of table.
This can cause problems in finding file in case of insert -> rename -> drop .
The signature should get used as the 'key' for finding the file.
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 205)
<https://reviews.apache.org/r/54826/#comment230774>
can you add a log message to indicate that this has started. Since this would typically be run only once in hour or so , INFO level should be good IMO
metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java (line 225)
<https://reviews.apache.org/r/54826/#comment230775>
can you add a debug level log message for when files get deleted ?
- Thejas Nair
On Dec. 16, 2016, 11:14 p.m., Daniel Dai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54826/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2016, 11:14 p.m.)
>
>
> Review request for hive and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See HIVE-15448
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9064e49
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java f7b2ed7
> metastore/src/java/org/apache/hadoop/hive/metastore/ReplChangeManager.java PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 6aca1b7
>
> Diff: https://reviews.apache.org/r/54826/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Daniel Dai
>
>