Adds FateOperation type #5218

kevinrr888 · 2024-12-30T18:49:28Z

Adds FateOperation enum. This consolidates all fate operations under one type (those used in thrift and those passed directly to a Fate object (outside of thrift)). This avoids the use of String here.

Adds a FateOperation enum which includes all current fate operations
Renamed existing thrift type FateOperation to TFateOperation
FateOperation includes all TFateOperations and fate operations performed outside of thrift (one example is COMMIT_COMPACTION)
FateOperation is now the type passed around instead of a String

I also verified that all FATE tests and all modified tests still pass and that sunny build passes
This is a prereq for #5130

Adds `FateOperation` enum. This consolidates all fate operations under one type (those used in thrift and those passed directly to a Fate object (outside of thrift)). This avoids the use of String here. - Adds a `FateOperation` enum which includes all current fate operations - Renamed existing thrift type `FateOperation` to `TFateOperation` - `FateOperation` includes all `TFateOperation`s and fate operations performed outside of thrift (one example is `COMMIT_COMPACTION`) - `FateOperation` is now the type passed around instead of a String

Deleted OBSOLETE_TABLE_BULK_IMPORT from TFateOperation but forgot to rebuild the thrift files after

dlmarion

Looks ok to me, but I'm not familiar with the serialization to have an opinion on the serialization changes that you made here.

dlmarion · 2025-01-02T18:35:17Z

core/src/main/java/org/apache/accumulo/core/fate/Fate.java

+    TABLE_SPLIT,
+    TABLE_TABLET_AVAILABILITY;
+
+    public static FateOperation fromThrift(TFateOperation tFateOp) {


Not sure if you have a test for this or not, but it would be good to add one that verifies that every TFateOperation has a FateOperation value and vice-versa. This would help with identifying additions / deletions in future releases.

They are not exactly the same. FateOperation has a few more types than TFateOperation. TFateOperation only includes the types used in thrift. Some operations are performed outside of thrift (for example see CompactionCoordinator.java).

Could probably still write some sort of test for this PR though.

It might look a little redundant, but I wonder if FateOperation should be FateOperation(TFateOperation) with null being an acceptable value. You could still write a test to ensure that all TFateOperations have an associated FateOperation.

I agree a test would be good here, you could write something that iterates over TFateOperation values and verifies there's a matching value in FateOperation as @dlmarion mentioned. Because FateOperation is a super set this would catch any future additions to TFateOperation that were not properly added to FateOperation without having to change the test. There's probably not a good way to test the other way though.

Thanks for the suggestions @dlmarion and @cshannon. I will add a test for this on Monday.

I wonder if FateOperation should be FateOperation(TFateOperation) with null being an acceptable value

I think this is a good way to connect the two more explicitly. I added this and the test in 30e4753

dlmarion · 2025-01-02T18:47:12Z

core/src/main/thrift/manager.thrift

@@ -58,7 +58,6 @@ enum FateOperation {
  TABLE_OFFLINE
  TABLE_MERGE
  TABLE_DELETE_RANGE
-  OBSOLETE_TABLE_BULK_IMPORT


Was this BulkImportV1?

I would assume so, but not 100% sure. It is no longer used in the code, so it was safely removed.

I think this was left here because thrift will send an integer corresponding to the enum over the wire for RPC calls. For example it will currently send 9 for TABLE_COMPACT. After this change it will send 8 for TABLE_COMPACT. Accumulo does not support RPC compatibility across major versions, however this change could cause misleading error message when using different versions of accumulo on each side of the RPC. Thrift supports specifying the integer directly in the IDL, we could leverage this to remove the enum and keep the integers stable.

enum FateOperation { TABLE_CREATE = 0, TABLE_CLONE = 1, TABLE_DELETE = 2, TABLE_RENAME = 3, TABLE_ONLINE = 4, TABLE_OFFLINE = 5, TABLE_MERGE = 6, TABLE_DELETE_RANGE = 7, // 8 was bulk v1 that was removed TABLE_COMPACT = 9, TABLE_IMPORT = 10, TABLE_EXPORT = 11, TABLE_CANCEL_COMPACT = 12, NAMESPACE_CREATE = 13, NAMESPACE_DELETE = 14, NAMESPACE_RENAME = 15, TABLE_BULK_IMPORT2 = 16, TABLE_TABLET_AVAILABILITY = 17, TABLE_SPLIT = 18 }

Keeping the integers stable could make debugging usage of different version less confusing (it is not supported, but it happens).

kevinrr888 · 2025-01-02T19:34:43Z

Looks ok to me, but I'm not familiar with the serialization to have an opinion on the serialization changes that you made here.

After you mentioned this, I looked into my serialization changes again. I made these changes under the assumption that Strings are no longer used for any of the TxInfos, but this isn't correct (I thought only TxInfo.TxName was stored as a String). The changes I have still work, but serializing/deserializing differently based on if it is a String was probably added for a reason and therefore should probably be kept.

I'll revert these changes.

cshannon

This generally looks pretty good to me but I am wondering if we should rename things from txName to be something different like txOperation. Now that the type is FateOperation it might be more clear.

However, the schema still uses the old name of txname and inside the enum for TxInfo it's still called TX_NAME as well so it would probably only be a good idea to change it if we renamed it everywhere to be consistent. Zookeeper specific code probably uses txname as well.

We don't have to rename it, but I figured I'd bring it up and it could also be a follow on too.

kevinrr888 · 2025-01-03T21:19:20Z

@cshannon - I considered the same and is what I was initially doing when first writing these changes (I think I was going with fateOp as the name). I decided to back out this rename due to many places needing a rename and wanted to keep this PR as simple as possible to get it merged in quickly.

I also think it would be good to rename this. I can do that in a follow-on PR after this is merged in.

core/src/main/java/org/apache/accumulo/core/fate/Fate.java

keith-turner · 2025-01-06T19:29:01Z

core/src/main/thrift/manager.thrift

@@ -58,7 +58,6 @@ enum FateOperation {
  TABLE_OFFLINE
  TABLE_MERGE
  TABLE_DELETE_RANGE
-  OBSOLETE_TABLE_BULK_IMPORT


I think this was left here because thrift will send an integer corresponding to the enum over the wire for RPC calls. For example it will currently send 9 for TABLE_COMPACT. After this change it will send 8 for TABLE_COMPACT. Accumulo does not support RPC compatibility across major versions, however this change could cause misleading error message when using different versions of accumulo on each side of the RPC. Thrift supports specifying the integer directly in the IDL, we could leverage this to remove the enum and keep the integers stable.

enum FateOperation { TABLE_CREATE = 0, TABLE_CLONE = 1, TABLE_DELETE = 2, TABLE_RENAME = 3, TABLE_ONLINE = 4, TABLE_OFFLINE = 5, TABLE_MERGE = 6, TABLE_DELETE_RANGE = 7, // 8 was bulk v1 that was removed TABLE_COMPACT = 9, TABLE_IMPORT = 10, TABLE_EXPORT = 11, TABLE_CANCEL_COMPACT = 12, NAMESPACE_CREATE = 13, NAMESPACE_DELETE = 14, NAMESPACE_RENAME = 15, TABLE_BULK_IMPORT2 = 16, TABLE_TABLET_AVAILABILITY = 17, TABLE_SPLIT = 18 }

Keeping the integers stable could make debugging usage of different version less confusing (it is not supported, but it happens).

kevinrr888 self-assigned this Dec 30, 2024

kevinrr888 added this to the 4.0.0 milestone Dec 30, 2024

Fix thrift build error:

8aa8d77

Deleted OBSOLETE_TABLE_BULK_IMPORT from TFateOperation but forgot to rebuild the thrift files after

dlmarion reviewed Jan 2, 2025

View reviewed changes

dlmarion requested a review from keith-turner January 2, 2025 18:51

Revert serialization changes

ee7056a

cshannon approved these changes Jan 3, 2025

View reviewed changes

FateOperation enum refactor, new FateOperationTest

30e4753

dlmarion reviewed Jan 6, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/fate/Fate.java Show resolved Hide resolved

dlmarion approved these changes Jan 6, 2025

View reviewed changes

keith-turner approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds FateOperation type #5218

Adds FateOperation type #5218

kevinrr888 commented Dec 30, 2024

dlmarion left a comment

dlmarion Jan 2, 2025

kevinrr888 Jan 2, 2025

dlmarion Jan 2, 2025

cshannon Jan 3, 2025

kevinrr888 Jan 3, 2025

kevinrr888 Jan 6, 2025

dlmarion Jan 2, 2025

kevinrr888 Jan 2, 2025

keith-turner Jan 6, 2025

kevinrr888 commented Jan 2, 2025 •

edited

Loading

cshannon left a comment •

edited

Loading

kevinrr888 commented Jan 3, 2025 •

edited

Loading

keith-turner Jan 6, 2025

Adds FateOperation type #5218

Are you sure you want to change the base?

Adds FateOperation type #5218

Conversation

kevinrr888 commented Dec 30, 2024

dlmarion left a 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

Choose a reason for hiding this comment

kevinrr888 commented Jan 2, 2025 • edited Loading

cshannon left a comment • edited Loading

Choose a reason for hiding this comment

kevinrr888 commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

kevinrr888 commented Jan 2, 2025 •

edited

Loading

cshannon left a comment •

edited

Loading

kevinrr888 commented Jan 3, 2025 •

edited

Loading