Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PHOENIX-7115 Create separate handler thread pool for invalidating server metadata cache #1748

Merged
merged 5 commits into from
Dec 22, 2023
Merged

PHOENIX-7115 Create separate handler thread pool for invalidating server metadata cache #1748

merged 5 commits into from
Dec 22, 2023

Conversation

shahrs87
Copy link
Contributor

@shahrs87 shahrs87 commented Dec 4, 2023

No description provided.

@shahrs87
Copy link
Contributor Author

shahrs87 commented Dec 5, 2023

@palashc Can you please review this?

palashc
palashc approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@palashc palashc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a few check-style fixes, but LGTM otherwise. Thank you!

jpisaac
jpisaac reviewed Dec 7, 2023
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java Outdated
@@ -604,6 +608,8 @@ public static PName newPName(byte[] keyBuffer, int keyOffset, int keyLength) {

private MetricsMetadataSource metricsSource;
private long metadataCacheInvalidationTimeoutMs;
private Connection invalidateMetadataCacheConnection = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahrs87 As discussed offline, wondering whether this connection for invalidation can be wrapped inside CQSI. It might be useful to provide an interface through CQSI. This will be in line with our current packaging of hConnections inside CQSI. Another advantage will be we can then use PhoenixConnection/CQSI to invoke this from out-of-band tools and clients too.
To begin with, the invalidation API can be exposed only for server-side connection and be a no-op for other clients/profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After making this change here, there are few test failures in:

  1. BackwardCompatibilityIT.testSelectUpsertWithNewClientWithMaxLookBackAge#org.apache.phoenix:phoenix-client-hbase-2.4:5.1.0
  2. org.apache.phoenix.end2end.LoadSystemTableSnapshotIT#testPhoenixUpgrade
    See the failing jenkins job here.
    From the jenkins logs, I am not able to root cause the failure. @jpisaac Can you please help here?

tkhurana
tkhurana reviewed Dec 7, 2023
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated
Configuration clonedConfiguration = PropertiesUtil.cloneConfig(this.config);
clonedConfiguration.setClass(CUSTOM_CONTROLLER_CONF_KEY,
InvalidateMetadataCacheControllerFactory.class, RpcControllerFactory.class);
invalidateMetadataCacheConnection = ConnectionFactory.createConnection(clonedConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openConnection() method uses HBaseFactoryProvider.getHConnectionFactory()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored openConnection and closeConnection methods here.

jpisaac
jpisaac reviewed Dec 15, 2023
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java Outdated
@@ -2090,6 +2090,8 @@ public void createTable(RpcController controller, CreateTableRequest request,
schemaName = rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
tableName = rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
fullTableName = SchemaUtil.getTableName(schemaName, tableName);
LOGGER.info("RSS create table, tenantID {}, schemaName: {}, table name: {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Was this for debug purposes?

jpisaac
jpisaac reviewed Dec 15, 2023
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
CUSTOM_CONTROLLER_CONF_KEY to instantiate InvalidateMetadataCacheController which has
a special priority for invalidate metadata cache operations.
*/
public Connection getInvalidateMetadataCacheConnection() throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Refactoring suggestions -
Maybe the initialization of the invalidateMetadataCacheConnection can be done in CQSI init itself, since these are one time actions. Instead of late binding/initialization. You may be able to simplify the connection creation without cloning the config and overriding the CUSTOM_CONTROLLER_CONF_KEY. See ServerSideRPCControllerFactory usage for eg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shahrs87 for pointing out the need for overriding the CUSTOM_CONTROLLER_CONF_KEY. Since we are going to late binding, may make sense to provide some synchronization, as it probably will be invoked via multiple threads and then we may have some leaky hConnections.

jpisaac
jpisaac approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@jpisaac jpisaac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some general refactoring suggestions. Also maybe refactor the invalidateServerMetadataCache method itself into CQSI as we discussed offline.

@shahrs87
Copy link
Contributor Author

Also maybe refactor the invalidateServerMetadataCache method itself into CQSI as we discussed offline.

@jpisaac Can you please review the patch once again? I have moved invalidateServerMetadataCache to CQSI completely. Thank you for the feedback.

shahrs87
shahrs87 commented Dec 21, 2023
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
@@ -3546,9 +3586,6 @@ public Void call() throws Exception {
success = true;
return null;
}
nSequenceSaltBuckets = ConnectionQueryServicesImpl.this.props.getInt(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving initialization of nSequenceSaltBuckets to the constructor since SKIP_SYSTEM_TABLES_EXISTENCE_CHECK can be set to true and we return early at L3587 and this will leave nSequenceSaltBuckets initiliazed to default value i.e 0.

@shahrs87
Copy link
Contributor Author

All the 3 failing tests are known flappers.

org.apache.phoenix.end2end.QueryTimeoutIT.testQueryTimeout
org.apache.phoenix.jdbc.LoggingHAConnectionLimiterIT.testActivityLogsOnUpsertWhenFailures
org.apache.phoenix.jdbc.ParallelPhoenixConnectionIT.testMetrics

I am merging this patch since all the review comments are addressed. Thank you @jpisaac @palashc for the reviews.

@shahrs87 shahrs87 merged commit 83134d9 into apache:PHOENIX-6883-feature Dec 22, 2023
0 of 3 checks passed
@palashc palashc mentioned this pull request May 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkhurana tkhurana tkhurana left review comments

@jpisaac jpisaac jpisaac approved these changes

@palashc palashc palashc approved these changes

@virajjasani virajjasani Awaiting requested review from virajjasani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shahrs87 @tkhurana @jpisaac @palashc