[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Hudson (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16438187#comment-16438187
 ] 

Hudson commented on PHOENIX-4575:
-

FAILURE: Integrated in Jenkins build Phoenix-4.x-HBase-0.98 #1858 (See 
[https://builds.apache.org/job/Phoenix-4.x-HBase-0.98/1858/])
PHOENIX-4575 Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be 
(jtaylor: rev fce9af53423d1804f5dc8daf8f28f9f7b38bbff5)
* (edit) phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
* (edit) 
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
* (edit) 
phoenix-core/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java
* (edit) 
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java
* (edit) phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java
* (edit) 
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java
* (edit) phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java


> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Fix For: 4.14.0, 5.0.0
>
> Attachments: PHOENIX-4575.patch, PHOENIX-4575.patch, 
> PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16437579#comment-16437579
 ] 

ASF GitHub Bot commented on PHOENIX-4575:
-

Github user JamesRTaylor commented on the issue:

https://github.com/apache/phoenix/pull/297
  
+1. Thanks for taking this to the finish line, @ChinmaySKulkarni. Is this 
on top of your patch for PHOENIX-4579? What order should I commit them?


> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16436945#comment-16436945
 ] 

ASF GitHub Bot commented on PHOENIX-4575:
-

Github user ChinmaySKulkarni commented on the issue:

https://github.com/apache/phoenix/pull/297
  
@JamesRTaylor rebased. @ankitsinghal phoenix build seems to be failing 
after commit 70c9c6bb2d2073bb40640a0a794032b166c4b63b for PHOENIX-4496. Please 
take a look.


> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Chinmay Kulkarni (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16435075#comment-16435075
 ] 

Chinmay Kulkarni commented on PHOENIX-4575:
---

Sure [~jamestaylor]. Please review my PR. Thanks!

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16435073#comment-16435073
 ] 

ASF GitHub Bot commented on PHOENIX-4575:
-

Github user ChinmaySKulkarni commented on the issue:

https://github.com/apache/phoenix/pull/297
  
@JamesRTaylor please review. Thanks @mujtabachohan for your initial 
patches. 

- Added nit changes as suggested by James on JIRA
- Rebased with master
- @ankitsinghal please check that this does not break anything related to 
your recent contributions  corresponding to Query Log 
[PHOENIX-2715](https://issues.apache.org/jira/browse/PHOENIX-2715). 
Changes are made to SYSLOG ddl statement. "KEEP_DELETED_CELLS" is by 
default false now (before the default value was true).
- Ran all UTs in the that are part of PHOENIX-2715 and ran UTs that are 
relevant to sys table creation and upgrade -> All passed.



> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16435057#comment-16435057
 ] 

ASF GitHub Bot commented on PHOENIX-4575:
-

GitHub user ChinmaySKulkarni opened a pull request:

https://github.com/apache/phoenix/pull/297

PHOENIX-4575: Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be 
property driven

Adds configurable properties for KEEP_DELETED_CELLS and VERSIONS.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ChinmaySKulkarni/phoenix PHOENIX-4575

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/phoenix/pull/297.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #297


commit f501bfacde19282cb2084b3afefc52c51b6951cb
Author: Chinmay Kulkarni 
Date:   2018-04-12T07:29:50Z

PHOENIX-4575: Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be 
property driven

Adds configurable properties for KEEP_DELETED_CELLS and VERSIONS.




> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[James Taylor (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16434838#comment-16434838
 ] 

James Taylor commented on PHOENIX-4575:
---

[~ckulkarni] - would you mind taking this over? It's ready to go except for one 
minor nit above (and may need to be rebased). We can get this in with the other 
couple of JIRAs you've been working on.

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Chinmay Kulkarni
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[James Taylor (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16356198#comment-16356198
 ] 

James Taylor commented on PHOENIX-4575:
---

Looks good, except for this change, keep the call to 
{{scan.setMaxVersions();}}, but just use the empty argument version of it (b/c 
we want to process all versions):
{code}
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
@@ -223,7 +223,6 @@ public class UpgradeUtil {
 
 Scan scan = new Scan();
 scan.setRaw(true);
-scan.setMaxVersions(MetaDataProtocol.DEFAULT_MAX_META_DATA_VERSIONS);
 ResultScanner scanner = null;
 HTableInterface source = null;
 HTableInterface target = null;
@@ -696,7 +695,6 @@ public class UpgradeUtil {
 boolean success = false;
 Scan scan = new Scan();
 scan.setRaw(true);
-
scan.setMaxVersions(MetaDataProtocol.DEFAULT_MAX_META_DATA_VERSIONS);
 HTableInterface seqTable = 
conn.getQueryServices().getTable(PhoenixDatabaseMetaData.SYSTEM_SEQUENCE_NAME_BYTES);
 try {
 boolean committed = false;
{code}

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Mujtaba Chohan (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16356121#comment-16356121
 ] 

Mujtaba Chohan commented on PHOENIX-4575:
-

[~jamestaylor]

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Mujtaba Chohan (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349204#comment-16349204
 ] 

Mujtaba Chohan commented on PHOENIX-4575:
-

[~jamestaylor] FYI attached v2 patch.

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch, PHOENIX-4575_v2.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Mujtaba Chohan (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16348034#comment-16348034
 ] 

Mujtaba Chohan commented on PHOENIX-4575:
-

Thanks for the review [~jamestaylor]. I’ll work on the changes. 

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[James Taylor (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16347793#comment-16347793
 ] 

James Taylor commented on PHOENIX-4575:
---

Thanks for the patch. Here's some feedback:
- Can we define these new properties attributes in QueryServices and their 
defaults in QueryServicesOptions instead?
- How about instead of META_DATA_KEEP_DELETED_CELLS we use a name of 
DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB 
(phoenix.system.default.keep.deleted.cells) as the property name and 
DEFAULT_SYSTEM_KEEP_DELETED_CELLS = false?
- How about a name of DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB 
(phoenix.system.default.max.versions) as the property name and 
DEFAULT_SYSTEM_MAX_VERSIONS = 1?
- I think you want to set the value of KEEP_DELETED_CELLS to the value of the 
new DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB (i.e. you don't want to set it to 
the string of property):
{code}
-HConstants.VERSIONS + "=" + 
MetaDataProtocol.DEFAULT_MAX_META_DATA_VERSIONS + ",\n" +
-HColumnDescriptor.KEEP_DELETED_CELLS + "="  + 
MetaDataProtocol.DEFAULT_META_DATA_KEEP_DELETED_CELLS + ",\n"+
+HConstants.VERSIONS + "=" + 
MetaDataProtocol.MAX_META_DATA_VERSIONS + ",\n" +
+HColumnDescriptor.KEEP_DELETED_CELLS + "="  + 
MetaDataProtocol.META_DATA_KEEP_DELETED_CELLS + ",\n"+
{code}
- Instead, put %s placeholders in the QueryConstant constant like this:
{code}
public static final String CREATE_TABLE_METADATA =
// Do not use IF NOT EXISTS as we sometimes catch the 
TableAlreadyExists
// exception and add columns to the SYSTEM.TABLE dynamically.
"CREATE TABLE " + SYSTEM_CATALOG_SCHEMA + ".\"" + 
SYSTEM_CATALOG_TABLE + "\"(\n" +
...
HConstants.VERSIONS + "=" + %s + ",\n" +
HColumnDescriptor.KEEP_DELETED_CELLS + "="  + %s + ",\n" +
...
{code}
and modify this method in ConnectionQueryServiceImpl (and 
ConnectionlessQueryServiceImpl):
{code}
// Available for testing
protected String getSystemCatalogDML() {
return String.format(QueryConstants.CREATE_TABLE_METADATA, 
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB, 
DEFAULT_SYSTEM_MAX_VERSIONS),
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB, 
DEFAULT_SYSTEM_KEEP_DELETED_CELLS));
}
{code}
- Do the same as above for the CREATE_FUNCTION_METADATA constant.
- Remove KEEP_DELETED_CELLS and VERSIONS from CREATE_SEQUENCE_METADATA and 
CREATE_STATS_TABLE_METADATA as it doesn't make sense for keep deleted cells to 
be on for these tables.
- Remove this code from ConnectionQueryServicesImpl as it'll like cause issues 
(and doesn't make sense):
{code}
if(props.get(QueryServices.DEFAULT_KEEP_DELETED_CELLS_ATTRIB) != 
null){
columnDesc.setKeepDeletedCells(props.getBoolean(
QueryServices.DEFAULT_KEEP_DELETED_CELLS_ATTRIB, 
QueryServicesOptions.DEFAULT_KEEP_DELETED_CELLS));
}
{code}
- Remove the QueryServices.DEFAULT_KEEP_DELETED_CELLS_ATTRIB and 
QueryServicesOptions.DEFAULT_KEEP_DELETED_CELLS as they won't be referenced any 
more.
- Remove these properties from MetaDataProtocol:
{code}
public static final int DEFAULT_MAX_META_DATA_VERSIONS = 1000;
public static final boolean DEFAULT_META_DATA_KEEP_DELETED_CELLS = true;
public static final int DEFAULT_MAX_STAT_DATA_VERSIONS = 1;
public static final boolean DEFAULT_STATS_KEEP_DELETED_CELLS = false;
{code} 


> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Assignee: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (PHOENIX-4575) Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven

[Mujtaba Chohan (JIRA)]

[ 
https://issues.apache.org/jira/browse/PHOENIX-4575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16347730#comment-16347730
 ] 

Mujtaba Chohan commented on PHOENIX-4575:
-

Attached patch adds 2 new properties that can be set in hbase-site.xml: 
 # phoenix.metadata.keep.deleted.cells
 # phoenix.metadata.max.versions

 

> Phoenix metadata KEEP_DELETED_CELLS and VERSIONS should be property driven
> --
>
> Key: PHOENIX-4575
> URL: https://issues.apache.org/jira/browse/PHOENIX-4575
> Project: Phoenix
>  Issue Type: New Feature
>Reporter: Mujtaba Chohan
>Priority: Major
> Attachments: PHOENIX-4575.patch
>
>
> This is to cater for circumstances where we need to alter state of 
> KEEP_DELETED_CELLS/VERSION on Phoenix meta tables.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)