PHOENIX-7459 : Bootstrap stream metadata when CDC is enabled on a table #2033

palashc · 2024-12-03T08:24:21Z

Jira: PHOENIX-7459

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java

phoenix-core/src/it/java/org/apache/phoenix/end2end/CDCStreamIT.java

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

phoenix-core-client/src/main/java/org/apache/phoenix/util/CDCUtil.java

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java

virajjasani

+1, pending clean build results

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

haridsv · 2024-12-06T05:02:59Z

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+                    .setMessage(ioe.getMessage())
+                    .setSchemaName(SYSTEM_SCHEMA_NAME)
+                    .setTableName(SYSTEM_TASK_TABLE).build().buildException();
+        }


Shouldn't we do this before creating the CDC object itself to be technically correct and avoid the window when the CDC is active but the stream is not? Also, how do we plan to synchronize this with the ongoing splits and merges?

@haridsv PHOENIX-7460 for keeping region metadata in sync.

haridsv · 2024-12-06T05:42:35Z

When will the DROP CDC be hooked up to change the stream status?

palashc · 2024-12-06T08:15:02Z

When will the DROP CDC be hooked up to change the stream status?

@haridsv Since DROP CDC needs more changes like not dropping the index but only stopping the writes to the index, I was thinking we can do that part as a separate JIRA. I can create another subtask under PHOENIX-7456, @virajjasani wdyt?

palashc · 2024-12-09T13:53:14Z

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java

+        } catch (Throwable t) {
+            LOGGER.error("Exception while bootstrapping CDC Stream Partition Metadata for "
+                    + taskRecord.getTableName() + " and timestamp " + timestamp.toString(), t);
+            return new TaskRegionObserver.TaskResult(TaskRegionObserver.TaskResultCode.FAIL, t.toString());


TODO: Consider a RETRY strategy here.

@haridsv @virajjasani Not sure under what kind of failures, we should be retrying the task here?

I think it is safe to retry on all SQLException scenarios, which can only happen when there is an issue with the system table availability.

virajjasani · 2024-12-10T02:15:24Z

phoenix-core-client/src/main/java/org/apache/phoenix/query/QueryConstants.java

+            STREAM_NAME + " VARCHAR NOT NULL," +
            // Non-PK columns
-            STREAM_NAME + " VARCHAR,\n" +
+            STREAM_STATUS + " VARCHAR,\n" +


@palashc btw the query pattern is going to be:

Step 1: Get the active table stream from the metadata. SELECT STREAM_NAME FROM SYSTEM.CDC_STREAM_STATUS WHERE TABLE_NAME = <table-name> AND STREAM_STATUS = 'ACTIVE'

How would keeping STREAM_NAME help here? I missed this earlier.

@virajjasani I changed it because the update pattern is

UPSERT (table_name, stream_name, ENABLING)

UPSERT (table_name, stream_name, ENABLED)

For querying, since there should only be one stream per table, the query should still be efficient?
SELECT STREAM_NAME FROM SYSTEM.CDC_STREAM_STATUS WHERE TABLE_NAME = <table-name> AND STREAM_STATUS = 'ENABLED'

Also for the fact that you can can have multiple streams in the same state, see my latest comment on the corresponding thread in the design doc.

haridsv

Since DROP CDC needs more changes like not dropping the index but only stopping the writes to the index, I was thinking we can do that part as a separate JIRA.

I feel we should include the DROP CDC change to complete the stream status change such that the new test to ensure only one active CDC can exist at any time can be extended to cover the scenario of creating a new CDC after dropping the existing one.

The follow up PR can then focus solely on reusing a single index instead of creating one per CDC. WDYT?

haridsv · 2024-12-10T10:06:02Z

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+                        statement.getDataTable().getTableName());
+
+        // for now, only track stream partition metadata for tables, TODO: updatable views
+        if (PTableType.TABLE.equals(dataTable.getType())) {


nit: with enums you can do equality check

Suggested change

if (PTableType.TABLE.equals(dataTable.getType())) {

if (PTableType.TABLE == dataTable.getType()) {

virajjasani · 2024-12-10T15:32:48Z

DROP CDC can be separate sub task IMO. Keeping individual sub tasks make it much simpler to follow the change history.
I think we should be good to go unless there is any significant concern.

virajjasani · 2024-12-10T16:19:03Z

Though if the PR changes are manageable as single sub task, we can include them here as well. Not a problem. It's upto you @palashc.

haridsv · 2024-12-11T06:44:49Z

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+                    .setTaskType(PTable.TaskType.CDC_STREAM_PARTITION)
+                    .setTableName(tableName) //give full table name
+                    .setSchemaName(streamName) // use schemaName to pass streamName
+                    .build());


Shouldn't we set the status to CREATED?

haridsv · 2024-12-11T08:14:45Z

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java

+                        .setStartTs(taskRecord.getTimeStamp())
+                        .setEndTs(null)
+                        .build());
+                LOGGER.error("Marking task as RETRY. " +


Shouldn't this be a warning, since we are not giving up yet?

haridsv · 2024-12-11T09:16:02Z

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java

+        try {
+            pconn = QueryUtil.getConnectionOnServer(env.getConfiguration()).unwrap(PhoenixConnection.class);
+            List<HRegionLocation> tableRegions = pconn.getQueryServices().getAllTableRegions(
+                    tableName.getBytes(), getTableRegionsTimeout);


If there is a timeout due to the table having a large number of regions, it may continue to timeout even in retries. How about we query for the PARTITION_END_KEY of the last partition and use getTableRegions to start from that key? We could load any existing partition records for this stream first (which will be empty the first time so we will default to HConstants.EMPTY_START_ROW), sort them and get the highest end key. The end key for getTableRegions would always be HConstants.EMPTY_START_ROW.

We can use existing method that is already used by getAllTableRegions() API.

haridsv · 2024-12-11T09:39:42Z

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+            ps.executeUpdate();
+            connection.commit();
+            LOGGER.info("Marked stream {} for table {} as DISABLED", streamName, parentTableName);
+        }


We should do this after the CDC drop is successful, may be even after the index drop.

phoenix-core-client/src/main/java/org/apache/phoenix/util/CDCUtil.java

initial implementation

f1acc89

palashc requested a review from virajjasani December 3, 2024 08:24

virajjasani reviewed Dec 3, 2024

View reviewed changes

...erver/src/main/java/org/apache/phoenix/coprocessor/tasks/CdcStreamPartitionMetadataTask.java Show resolved Hide resolved

phoenix-core/src/it/java/org/apache/phoenix/end2end/CDCStreamIT.java Show resolved Hide resolved

virajjasani reviewed Dec 3, 2024

View reviewed changes

review fixes

317cc48

virajjasani approved these changes Dec 4, 2024

View reviewed changes

add timeout for get all table regions

ac4b556

haridsv reviewed Dec 6, 2024

View reviewed changes

review fixes 2

315ca3b

palashc commented Dec 9, 2024

View reviewed changes

virajjasani reviewed Dec 10, 2024

View reviewed changes

allow only one stream per table

3eeea08

haridsv reviewed Dec 10, 2024

View reviewed changes

more changes

d398cfd

palashc requested review from virajjasani and haridsv December 10, 2024 19:33

haridsv reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-7459 : Bootstrap stream metadata when CDC is enabled on a table #2033

PHOENIX-7459 : Bootstrap stream metadata when CDC is enabled on a table #2033

palashc commented Dec 3, 2024 •

edited by virajjasani

Loading

virajjasani left a comment

haridsv Dec 6, 2024

palashc Dec 6, 2024

haridsv commented Dec 6, 2024

palashc commented Dec 6, 2024 •

edited

Loading

palashc Dec 9, 2024

palashc Dec 10, 2024

haridsv Dec 10, 2024

palashc Dec 10, 2024

virajjasani Dec 10, 2024

palashc Dec 10, 2024 •

edited

Loading

haridsv Dec 10, 2024

haridsv left a comment

haridsv Dec 10, 2024

virajjasani commented Dec 10, 2024

virajjasani commented Dec 10, 2024 •

edited

Loading

haridsv Dec 11, 2024

haridsv Dec 11, 2024

haridsv Dec 11, 2024 •

edited

Loading

virajjasani Dec 12, 2024

haridsv Dec 11, 2024

	if (PTableType.TABLE.equals(dataTable.getType())) {
	if (PTableType.TABLE == dataTable.getType()) {

PHOENIX-7459 : Bootstrap stream metadata when CDC is enabled on a table #2033

Are you sure you want to change the base?

PHOENIX-7459 : Bootstrap stream metadata when CDC is enabled on a table #2033

Conversation

palashc commented Dec 3, 2024 • edited by virajjasani Loading

virajjasani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haridsv commented Dec 6, 2024

palashc commented Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashc Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haridsv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajjasani commented Dec 10, 2024

virajjasani commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haridsv Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashc commented Dec 3, 2024 •

edited by virajjasani

Loading

palashc commented Dec 6, 2024 •

edited

Loading

palashc Dec 10, 2024 •

edited

Loading

virajjasani commented Dec 10, 2024 •

edited

Loading

haridsv Dec 11, 2024 •

edited

Loading