You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "kadirozde (via GitHub)" <gi...@apache.org> on 2024/05/26 15:02:22 UTC
[PR] PHOENIX-7314 Phoenix TTL for minor compaction [phoenix]
kadirozde opened a new pull request, #1896:
URL: https://github.com/apache/phoenix/pull/1896
(no comment)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Phoenix TTL for minor compaction [phoenix]
Posted by "kadirozde (via GitHub)" <gi...@apache.org>.
kadirozde commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1615326932
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java:
##########
@@ -1654,7 +1655,8 @@ else if (linkType == PHYSICAL_TABLE) {
else {
byte[] maxLookbackAgeInBytes = result.getValue(TABLE_FAMILY_BYTES, MAX_LOOKBACK_AGE_BYTES);
if (maxLookbackAgeInBytes != null) {
- return PLong.INSTANCE.getCodec().decodeLong(maxLookbackAgeInBytes, 0, SortOrder.getDefault());
+ return PInteger.INSTANCE.getCodec().decodeInt(maxLookbackAgeInBytes, 0,
Review Comment:
That is great! Thank you very much! I will update the PR accordingly.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Phoenix TTL for minor compaction [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1615288652
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java:
##########
@@ -1654,7 +1655,8 @@ else if (linkType == PHYSICAL_TABLE) {
else {
byte[] maxLookbackAgeInBytes = result.getValue(TABLE_FAMILY_BYTES, MAX_LOOKBACK_AGE_BYTES);
if (maxLookbackAgeInBytes != null) {
- return PLong.INSTANCE.getCodec().decodeLong(maxLookbackAgeInBytes, 0, SortOrder.getDefault());
+ return PInteger.INSTANCE.getCodec().decodeInt(maxLookbackAgeInBytes, 0,
Review Comment:
The test in MaxLookbackIT seems to be failing as MAX_LOOKBACK_AGE is of type BIGINT in SYSCAT while we are decoding it via PInteger decoder instead of PLong decoder. I think we need to change the type of MAX_LOOKBACK_AGE column in SYSCAT to INTERGER.
I am also checking other places where we might we using max lookback as long and will update on comment. 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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "kadirozde (via GitHub)" <gi...@apache.org>.
kadirozde merged PR #1896:
URL: https://github.com/apache/phoenix/pull/1896
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "kadirozde (via GitHub)" <gi...@apache.org>.
kadirozde commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1615887838
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java:
##########
@@ -581,92 +601,135 @@ private boolean areMutationsInSameTable(Table targetHTable, Region region) {
region.getTableDescriptor().getTableName().getName()) == 0);
}
+ @Override
+ public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+ InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException {
+ if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {
+ return scanner;
+ } else {
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override public InternalScanner run() throws Exception {
+ String tableName = c.getEnvironment().getRegion().getRegionInfo().getTable()
+ .getNameAsString();
+ long maxLookbackInMillis =
+ UngroupedAggregateRegionObserver.getMaxLookbackInMillis(
+ tableName, store.getColumnFamilyName(),
+ BaseScannerRegionObserverConstants.getMaxLookbackInMillis(
+ c.getEnvironment().getConfiguration()));
+ maxLookbackInMillis = CompactionScanner.getMaxLookbackInMillis(tableName,
Review Comment:
The one in UngroupedAggregateRegionObserver is for caching the max lookback age in order to prevent getTable calls in the coproc flush hook. The other is for overriding the cached value by other coprocs, (for example a backup coproc).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Phoenix TTL for minor compaction [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1615288652
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java:
##########
@@ -1654,7 +1655,8 @@ else if (linkType == PHYSICAL_TABLE) {
else {
byte[] maxLookbackAgeInBytes = result.getValue(TABLE_FAMILY_BYTES, MAX_LOOKBACK_AGE_BYTES);
if (maxLookbackAgeInBytes != null) {
- return PLong.INSTANCE.getCodec().decodeLong(maxLookbackAgeInBytes, 0, SortOrder.getDefault());
+ return PInteger.INSTANCE.getCodec().decodeInt(maxLookbackAgeInBytes, 0,
Review Comment:
The test in MaxLookbackIT seems to be failing as MAX_LOOKBACK_AGE is of type BIGINT in SYSCAT while we are decoding it via PInteger decoder instead of PLong decoder. I think we need to change the type of MAX_LOOKBACK_AGE column in SYSCAT to INTERGER. I tested the IT with MAX_LOOKBACK_AGE as INT column in SYSCAT and it worked.
I am also checking other places where we might we using max lookback as long and will update on comment. 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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1619835351
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/CompactionScanner.java:
##########
@@ -419,13 +551,15 @@ private void formNextCompactionRowVersion(LinkedList<LinkedList<Cell>> columns,
if (cell.getTimestamp() >= rowContext.maxTimestamp) {
rowContext.familyDeleteMarker = cell;
column.removeFirst();
+ break;
}
continue;
}
else if (cell.getType() == Cell.Type.DeleteFamilyVersion) {
- if (cell.getTimestamp() >= rowContext.maxTimestamp) {
+ if (cell.getTimestamp() == rowVersion.ts) {
Review Comment:
Nice
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "kadirozde (via GitHub)" <gi...@apache.org>.
kadirozde commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1616049951
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java:
##########
@@ -581,92 +601,135 @@ private boolean areMutationsInSameTable(Table targetHTable, Region region) {
region.getTableDescriptor().getTableName().getName()) == 0);
}
+ @Override
+ public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+ InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException {
+ if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {
+ return scanner;
+ } else {
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override public InternalScanner run() throws Exception {
+ String tableName = c.getEnvironment().getRegion().getRegionInfo().getTable()
+ .getNameAsString();
+ long maxLookbackInMillis =
+ UngroupedAggregateRegionObserver.getMaxLookbackInMillis(
Review Comment:
Yes, you are right that this solution here is eventual consistent. The precompact is called for minor and major compaction. Yes, major compaction is done periodically typically once a week. However, the compaction policy may decide to skip it. Minor compaction is supposed to run whenever the number of HFiles exceeds the configured threshold. If the eventual consistency is fine for data table regions, then eventual consistency between data table region and index from the max lookback perspective should be also fine. However, I think we need a better solution than this such that we need to populate and invalidate this cache whenever the max lookback config changes. The solution here is a stopgap. Hopefully, we will implement a better solution before 5.3. Please note we can continue to configure "phoenix.max.lookback.age.seconds" at the table level to using the Phoenix alter table command to get much better eventual consistency meanwhile.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#discussion_r1615843022
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java:
##########
@@ -581,92 +601,135 @@ private boolean areMutationsInSameTable(Table targetHTable, Region region) {
region.getTableDescriptor().getTableName().getName()) == 0);
}
+ @Override
+ public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+ InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException {
+ if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {
+ return scanner;
+ } else {
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override public InternalScanner run() throws Exception {
+ String tableName = c.getEnvironment().getRegion().getRegionInfo().getTable()
+ .getNameAsString();
+ long maxLookbackInMillis =
+ UngroupedAggregateRegionObserver.getMaxLookbackInMillis(
Review Comment:
This guarantees eventual consistency and the static map is filled up by pre-compact hook. So that means once DDL to change max lookback for a table is run, new value will start getting used on region server (updated in static map) when pre-compact hook on that table is called. So, I have following doubts:
- Is there a set frequency within which pre-compact will get called at least once for each table on each region server hosting regions of that table?
- For short duration within the max lookback map just after changing max lookback, value of max lookback for table and index/view and view index might not match, is that fine?
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java:
##########
@@ -581,92 +601,135 @@ private boolean areMutationsInSameTable(Table targetHTable, Region region) {
region.getTableDescriptor().getTableName().getName()) == 0);
}
+ @Override
+ public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+ InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException {
+ if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {
+ return scanner;
+ } else {
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override public InternalScanner run() throws Exception {
+ String tableName = c.getEnvironment().getRegion().getRegionInfo().getTable()
+ .getNameAsString();
+ long maxLookbackInMillis =
+ UngroupedAggregateRegionObserver.getMaxLookbackInMillis(
+ tableName, store.getColumnFamilyName(),
+ BaseScannerRegionObserverConstants.getMaxLookbackInMillis(
+ c.getEnvironment().getConfiguration()));
+ maxLookbackInMillis = CompactionScanner.getMaxLookbackInMillis(tableName,
Review Comment:
Why are we maintaining two maps of max lookback age: one in this class and other in Compaction Scanner?
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java:
##########
@@ -581,92 +601,135 @@ private boolean areMutationsInSameTable(Table targetHTable, Region region) {
region.getTableDescriptor().getTableName().getName()) == 0);
}
+ @Override
+ public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+ InternalScanner scanner, FlushLifeCycleTracker tracker) throws IOException {
+ if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {
+ return scanner;
+ } else {
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override public InternalScanner run() throws Exception {
+ String tableName = c.getEnvironment().getRegion().getRegionInfo().getTable()
+ .getNameAsString();
+ long maxLookbackInMillis =
+ UngroupedAggregateRegionObserver.getMaxLookbackInMillis(
+ tableName, store.getColumnFamilyName(),
+ BaseScannerRegionObserverConstants.getMaxLookbackInMillis(
+ c.getEnvironment().getConfiguration()));
+ maxLookbackInMillis = CompactionScanner.getMaxLookbackInMillis(tableName,
+ store.getColumnFamilyName(), maxLookbackInMillis);
+ return new CompactionScanner(c.getEnvironment(), store, scanner,
+ maxLookbackInMillis, null, null, false, true);
+ }
+ });
+ }
+ }
+
@Override
public InternalScanner preCompact(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
InternalScanner scanner, ScanType scanType, CompactionLifeCycleTracker tracker,
CompactionRequest request) throws IOException {
- if (scanType.equals(ScanType.COMPACT_DROP_DELETES)) {
- final TableName tableName = c.getEnvironment().getRegion().getRegionInfo().getTable();
- // Compaction and split upcalls run with the effective user context of the requesting user.
- // This will lead to failure of cross cluster RPC if the effective user is not
- // the login user. Switch to the login user context to ensure we have the expected
- // security context.
- return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
- @Override
- public InternalScanner run() throws Exception {
- InternalScanner internalScanner = scanner;
- if (request.isMajor()) {
- boolean isDisabled = false;
- boolean isMultiTenantIndexTable = false;
- if (tableName.getNameAsString().startsWith(MetaDataUtil.VIEW_INDEX_TABLE_PREFIX)) {
- isMultiTenantIndexTable = true;
- }
- final String fullTableName = isMultiTenantIndexTable ?
- SchemaUtil.getParentTableNameFromIndexTable(tableName.getNameAsString(),
- MetaDataUtil.VIEW_INDEX_TABLE_PREFIX) :
- tableName.getNameAsString();
- PTable table = null;
- try (PhoenixConnection conn = QueryUtil.getConnectionOnServer(
- compactionConfig).unwrap(PhoenixConnection.class)) {
- table = conn.getTableNoCache(fullTableName);
- } catch (Exception e) {
- if (e instanceof TableNotFoundException) {
- LOGGER.debug("Ignoring HBase table that is not a Phoenix table: "
- + fullTableName);
- // non-Phoenix HBase tables won't be found, do nothing
- } else {
- LOGGER.error(
- "Unable to modify compaction scanner to retain deleted "
- + "cells for a table with disabled Index; "
- + fullTableName, e);
- }
- }
- // The previous indexing design needs to retain delete markers and deleted
- // cells to rebuild disabled indexes. Thus, we skip major compaction for
- // them. GlobalIndexChecker is the coprocessor introduced by the current
- // indexing design.
- if (table != null &&
- !PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME.equals(fullTableName) &&
- !ServerUtil.hasCoprocessor(c.getEnvironment(),
- GlobalIndexChecker.class.getName())) {
- List<PTable>
- indexes =
- PTableType.INDEX.equals(table.getType()) ?
- Lists.newArrayList(table) :
- table.getIndexes();
- // FIXME need to handle views and indexes on views as well
- for (PTable index : indexes) {
- if (index.getIndexDisableTimestamp() != 0) {
- LOGGER.info("Modifying major compaction scanner to retain "
- + "deleted cells for a table with disabled index: "
- + fullTableName);
- isDisabled = true;
- break;
- }
- }
- }
- if (table != null && !isDisabled && isPhoenixTableTTLEnabled) {
- internalScanner =
- new CompactionScanner(c.getEnvironment(), store, scanner,
- MetaDataUtil.getMaxLookbackAge(
- c.getEnvironment().getConfiguration(), table.getMaxLookbackAge()),
- SchemaUtil.getEmptyColumnFamily(table),
- table.getEncodingScheme()
- == PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS ?
- QueryConstants.EMPTY_COLUMN_BYTES :
- table.getEncodingScheme().encode(QueryConstants.ENCODED_EMPTY_COLUMN_NAME)
- );
+
+ final TableName tableName = c.getEnvironment().getRegion().getRegionInfo().getTable();
+ // Compaction and split upcalls run with the effective user context of the requesting user.
+ // This will lead to failure of cross cluster RPC if the effective user is not
+ // the login user. Switch to the login user context to ensure we have the expected
+ // security context.
+ return User.runAsLoginUser(new PrivilegedExceptionAction<InternalScanner>() {
+ @Override
+ public InternalScanner run() throws Exception {
+ InternalScanner internalScanner = scanner;
+ boolean keepDeleted = false;
+ boolean isMultiTenantIndexTable = false;
+ if (tableName.getNameAsString().startsWith(MetaDataUtil.VIEW_INDEX_TABLE_PREFIX)) {
+ isMultiTenantIndexTable = true;
+ }
+ final String fullTableName = isMultiTenantIndexTable ?
+ SchemaUtil.getParentTableNameFromIndexTable(tableName.getNameAsString(),
+ MetaDataUtil.VIEW_INDEX_TABLE_PREFIX) :
+ tableName.getNameAsString();
+ PTable table = null;
+ Integer maxLookbackAge = null;
+ try (PhoenixConnection conn = QueryUtil.getConnectionOnServer(
+ compactionConfig).unwrap(PhoenixConnection.class)) {
+ table = conn.getTableNoCache(fullTableName);
+ maxLookbackAge = table.getMaxLookbackAge();
+ if (table.getType() == PTableType.INDEX) {
Review Comment:
I think we don't need this `if` check now as Indexes inherit max lookback of tables and view indexes inherit max lookback of parent view?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7314 Enable CompactionScanner for flushes and minor compaction [phoenix]
Posted by "kadirozde (via GitHub)" <gi...@apache.org>.
kadirozde commented on PR #1896:
URL: https://github.com/apache/phoenix/pull/1896#issuecomment-2136106662
I run the failed tests locally and all of them passes for me.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org