PHOENIX-6627 remove tephra references - WIP #1455

zsgyulavari · 2022-06-20T09:21:02Z

No description provided.

gjacoby126 · 2022-06-20T18:10:50Z

Thanks for taking this up, @zsgyulavari ! Please let me know if there's anything I can do to help -- it would be great if we could get this into 5.2 which is coming soon.

On the first pass it looks like there are 151 test failures, but most of them seem to be from the same problem in TransactionFactory$Provider's fromCode method. On line 69 it's making an assumption that the length of the Provider enum is equal to the largest Provider enum value (that is, there are 2 providers, and the highest provider is enum value 2.) That's not true anymore now that the Tephra provider is gone, so the logic has to change.

gjacoby126 · 2022-06-20T18:11:01Z

Also, what will happen to user tables who upgrade from Phoenix 5.1 -> 5.2 and have the Provider set to Tephra? Would that cause the region to crash from trying to load an enum value that doesn't exist? If so, what sort of upgrade logic should we include? I can think of three options:

Switch tables to non-transactional
Switch tables to Omid
Keep the Tephra enum and just WARN or ERROR on region load that it doesn't do anything

1 and 2 are both tricky to coordinate on a rolling restart, because the normal upgrade logic runs when SYSTEM.CATALOG is upgraded, but the crashing would probably come before that, when a region hosting a TEPHRA-enabled table has its region server upgraded. But I worry that some operators would miss the warning in their logs.

@stoty @jpisaac @kadirozde @virajjasani , curious on your opinions.

stoty · 2022-06-21T12:50:31Z

We could just fail all operations on the affected tables.
While drastic, this would make sure that the tables fail early, and there's no data corruption.

Has anyone ever seen a live Tephra user in production ?

gjacoby126 · 2022-06-21T18:01:01Z

@stoty - so long as the region comes online, I like the idea of the coproc just sending a clear, non-retriable exception if tephra is enabled on a region. (If the region crashes, then there's a risk of it taking down the entire cluster as the HMaster keeps trying to bring it up on other RSs, killing each one.)

And no, I've never seen a live Tephra use case in production.

stoty · 2022-08-09T04:49:17Z

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

@@ -987,10 +987,8 @@ public void testDivergedViewsStayDiverged() throws Exception {
    }

    @Test
+    @Ignore("tephra dependent test case")


Instead of @Ignore-ing this test, we could just change it to test for OmidTransactionaProcessor instead.

stoty · 2022-08-09T04:51:52Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ImmutableIndexIT.java

@@ -137,9 +137,7 @@ public static synchronized Collection<Object[]> data() {
 		        Arrays.asList(new Object[][] {
    				{ false, false, null, false }, { false, false, null, true },
    				{ false, true, "OMID", false },
-                    { false, true, "TEPHRA", false }, { false, true, "TEPHRA", true },
    				{ true, false, null, false }, { true, false, null, true },


Here we have a single combination that uses OMID, and four that uses Tephra (probably to save on execution time).
We should still have remove the Omid one, and change the Tephra ones to Omid instead (i.e., instead of five transactional combinations, we should have four)

Correction:
Those seem to be unsupported operations for Omid, so this is fine.

stoty · 2022-08-09T04:52:23Z

phoenix-core/src/it/java/org/apache/phoenix/tx/ParameterizedTransactionIT.java

@@ -305,9 +304,9 @@ public void testNonTxToTxTable() throws Exception {
        }

        htable = conn.unwrap(PhoenixConnection.class).getQueryServices().getTable(Bytes.toBytes( nonTxTableName));
-        assertTrue(htable.getDescriptor().getCoprocessors().contains(TephraTransactionalProcessor.class.getName()));
+        //assertTrue(htable.getDescriptor().getCoprocessors().contains(TephraTransactionalProcessor.class.getName()));


again, check for the Omid coprocessor instead.

stoty · 2022-08-09T04:53:29Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java

@@ -1255,7 +1255,7 @@ private PTable getTableFromCells(List<Cell> tableCellList, List<List<Cell>> allC
                            transactionalKv.getValueOffset(),
                            transactionalKv.getValueLength()))) {
                // For backward compat, prior to client setting TRANSACTION_PROVIDER
-                transactionProvider = TransactionFactory.Provider.TEPHRA;
+                transactionProvider = TransactionFactory.Provider.OMID;


This is a behaviour change for old clients that only expect Tephra to exist.
Maybe we should just error out in this case, as the expected functionality has been removed.

I am assuming from the comment the the Omid aware clients always set the Provider, but haven't checked it.

stoty · 2022-08-09T04:55:57Z

phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java

@@ -1867,7 +1867,7 @@ public static PTable createFromProto(PTableProtos.PTable table) {
            transactionProvider = TransactionFactory.Provider.fromCode(table.getTransactionProvider());
        } else if (table.hasTransactional()) {
            // For backward compatibility prior to transactionProvider field
-            transactionProvider = TransactionFactory.Provider.TEPHRA;
+            transactionProvider = TransactionFactory.Provider.OMID;


We should consider erroring out in this case as well.

stoty · 2022-08-09T05:01:07Z

phoenix-core/src/main/java/org/apache/phoenix/transaction/TransactionFactory.java

-        TEPHRA((byte)1, tephraTransactionProvider,
-            tephraTransactionProvider instanceof TephraTransactionProvider),
-        OMID((byte)2, OmidTransactionProvider.getInstance(), true);
+        OMID((byte)2, OmidTransactionProvider.getInstance());


This changes the ordinal, which SHOULD not be a real problem as we don't use that for serialization, but keeping TEPHRA, and just renaming it to something like TEPHRA_REMOVED would possibly also make it easier to handle the hairy cases where tephra defined for old tables.

stoty · 2022-08-09T05:02:36Z

phoenix-core/src/main/java/org/apache/phoenix/transaction/TransactionFactory.java

@@ -94,7 +66,7 @@ public static PhoenixTransactionProvider getTransactionProvider(byte[] txState,
            return null;
        }
        Provider provider = (clientVersion < MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0) 
-                ? Provider.TEPHRA
+                ? Provider.OMID


Again, should we just error out ?

stoty · 2022-08-09T05:03:05Z

phoenix-core/src/test/java/org/apache/phoenix/util/TestUtil.java

-        }
-        return filteredData;
+    	return data;
+    	//TODO: remove this method altogether


Yes, it shouldn't be too difficult.

stoty · 2022-08-09T05:04:58Z

phoenix-core/src/main/java/org/apache/phoenix/transaction/NotAvailableTransactionProvider.java

@@ -60,7 +60,7 @@ public PhoenixTransactionClient getTransactionClient(Configuration config, Conne

    @Override
    public Provider getProvider() {
-        return TransactionFactory.Provider.TEPHRA;
+        return TransactionFactory.Provider.OMID;


This is not right.
We should either return null, or the UnavailableProvider's type, if we keep that,

apurtell · 2022-08-10T21:39:10Z

I'm going to close this PR but we can reopen it if the alternative PR I am about to open is not acceptable for some reason.

PHOENIX-6627 remove tephra references

8608cb3

zsgyulavari changed the title ~~WIP: PHOENIX-6627 remove tephra references~~ PHOENIX-6627 remove tephra references - WIP Jun 20, 2022

zsgyulavari marked this pull request as draft June 20, 2022 09:32

stoty requested changes Aug 9, 2022

View reviewed changes

apurtell closed this Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-6627 remove tephra references - WIP #1455

PHOENIX-6627 remove tephra references - WIP #1455

zsgyulavari commented Jun 20, 2022

gjacoby126 commented Jun 20, 2022

gjacoby126 commented Jun 20, 2022

stoty commented Jun 21, 2022

gjacoby126 commented Jun 21, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

stoty Aug 9, 2022

apurtell commented Aug 10, 2022

PHOENIX-6627 remove tephra references - WIP #1455

PHOENIX-6627 remove tephra references - WIP #1455

Conversation

zsgyulavari commented Jun 20, 2022

gjacoby126 commented Jun 20, 2022

gjacoby126 commented Jun 20, 2022

stoty commented Jun 21, 2022

gjacoby126 commented Jun 21, 2022

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

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

apurtell commented Aug 10, 2022