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.

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

PHOENIX-7076 : MetaDataRegionObserver#postOpen hook improvements #1735

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

Divneet18
Copy link
Contributor

No description provided.

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

Left few nits

@@ -109,13 +112,16 @@ public class MetaDataRegionObserver implements RegionObserver,RegionCoprocessor
QueryConstants.SYSTEM_SCHEMA_NAME_BYTES,
PhoenixDatabaseMetaData.SYSTEM_CATALOG_TABLE_BYTES);
protected ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);
private ScheduledThreadPoolExecutor truncateTaskExectuor = new ScheduledThreadPoolExecutor(1,
new ThreadFactoryBuilder().setDaemon(true).setNameFormat("task-truncated%s").build());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: task-truncated-%d

@@ -158,6 +158,7 @@ public interface QueryServices extends SQLCloseable {
public static final String INDEX_FAILURE_HANDLING_REBUILD_INTERVAL_ATTRIB =
"phoenix.index.failure.handling.rebuild.interval";
public static final String INDEX_REBUILD_TASK_INITIAL_DELAY = "phoenix.index.rebuild.task.initial.delay";
public static final String INDEX_START_TRUNCATE_TASK_DELAY = "phoenix.index.start.truncate.task.delay";
Copy link
Contributor

Choose a reason for hiding this comment

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

stats truncate is not related to index, so let's remove "index" from configs everywhere

@@ -216,6 +216,7 @@ public class QueryServicesOptions {
public static final boolean DEFAULT_INDEX_FAILURE_THROW_EXCEPTION = true;
public static final long DEFAULT_INDEX_FAILURE_HANDLING_REBUILD_INTERVAL = 60000; // 60 secs
public static final long DEFAULT_INDEX_REBUILD_TASK_INITIAL_DELAY = 10000; // 10 secs
public static final long DEFAULT_INDEX_START_TRUNCATE_TASK_DELAY = 20000; // 20 secs
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -156,6 +162,10 @@ public void start(CoprocessorEnvironment env) throws IOException {
config.getLong(
QueryServices.INDEX_REBUILD_TASK_INITIAL_DELAY,
QueryServicesOptions.DEFAULT_INDEX_REBUILD_TASK_INITIAL_DELAY);
startTruncateTaskDelay =
config.getLong(
QueryServices.INDEX_START_TRUNCATE_TASK_DELAY,
Copy link
Contributor

Choose a reason for hiding this comment

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

what has this config got to do with index ?

private boolean enableRebuildIndex = QueryServicesOptions.DEFAULT_INDEX_FAILURE_HANDLING_REBUILD;
private long rebuildIndexTimeInterval = QueryServicesOptions.DEFAULT_INDEX_FAILURE_HANDLING_REBUILD_INTERVAL;
private static Map<PName, Long> batchExecutedPerTableMap = new HashMap<PName, Long>();
@GuardedBy("MetaDataRegionObserver.class")
private static Properties rebuildIndexConnectionProps;
// Added for test purposes
private long initialRebuildTaskDelay;
private long startTruncateTaskDelay;
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this to statsTruncateTaskDelay

if (env.getConfiguration()
.getBoolean(STATS_COLLECTION_ENABLED, DEFAULT_STATS_COLLECTION_ENABLED)) {
truncateTaskExectuor.schedule(r, statsTruncateTaskDelay, TimeUnit.MILLISECONDS);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an else clause and then you can add a log there "Stats collection is disabled"

@tkhurana tkhurana merged commit 5128ad0 into apache:master Nov 20, 2023
0 of 2 checks passed
Divneet18 added a commit to Divneet18/phoenix that referenced this pull request Dec 12, 2023
…che#1735)

* PHOENIX-7076 : MetaDataRegionObserver#postOpen hook improvements

* updated import and fixed checkstyle

* fixed checkstyle

* changed varialbe names and few other nits

* Added logger info

---------

Co-authored-by: divneet-kaur <divneet.kaur@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants