HDDS-11975. wrap TermIndex in ExecutionContext #7602

sumitagrawl · 2024-12-20T12:29:55Z

What changes were proposed in this pull request?

Wrap TermIndex in ExecutionContext. ExecutionContext.getIndex() will have same value as termIndex.

There is no logic change.

Later, with below Jira, dependency with ratisIndex will be removed,
https://issues.apache.org/jira/browse/HDDS-11901

Parent Jira:
https://issues.apache.org/jira/browse/HDDS-11897

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11975

How was this patch tested?

Existing testcase

adoroszlai

Thanks @sumitagrawl for the patch.

I think we can keep using TermIndex even if the index is not generated by Ratis, similar to how it's used currently when Ratis is disabled in OM. TermIndex is just an interface, we can define custom implementation, if additional data is needed for generating our own index. Thus we could avoid wrapping it in yet another object, and skip refactoring a bunch of code.

sumitagrawl · 2024-12-23T07:10:36Z

Thanks @sumitagrawl for the patch.

I think we can keep using TermIndex even if the index is not generated by Ratis, similar to how it's used currently when Ratis is disabled in OM. TermIndex is just an interface, we can define custom implementation, if additional data is needed for generating our own index. Thus we could avoid wrapping it in yet another object, and skip refactoring a bunch of code.

@adoroszlai ExecutionContext here have just one entry, but in new flow, it’s context for execution which will contains other members as required in future like both index and termIndex. Additional other parameters.
It’s problem to change interface for every new parameters.

devmadhuu

Thanks @sumitagrawl for the patch. Pls check comment.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java

adoroszlai · 2024-12-23T09:50:29Z

ExecutionContext here have just one entry, but in new flow, it’s context for execution which will contains other members as required in future like both index and termIndex. Additional other parameters. It’s problem to change interface for every new parameters.

in POC (HDDS-11418. leader executor framework [prototype] #7406) ExecutionContext has one additional member: batchOperation
ExecutionContext is just a holder for "stuff", with get/set methods for each member
index/termIndex and batchOperation are used in unrelated places
instances of ExecutionContext may or may not have these members set
RequestContext is also a holder for "stuff"
RequestContext even has entry for ExecutionContext

So I think ExecutionContext is unnecessary and prone to be misused.

Why not put additional other parameters (batchOperation for now) in RequestContext?

sumitagrawl · 2024-12-30T09:00:35Z

ExecutionContext here have just one entry, but in new flow, it’s context for execution which will contains other members as required in future like both index and termIndex. Additional other parameters. It’s problem to change interface for every new parameters.

in POC (HDDS-11418. leader executor framework [prototype] #7406) ExecutionContext has one additional member: batchOperation

ExecutionContext is just a holder for "stuff", with get/set methods for each member

index/termIndex and batchOperation are used in unrelated places

instances of ExecutionContext may or may not have these members set

RequestContext is also a holder for "stuff"

RequestContext even has entry for ExecutionContext

So I think ExecutionContext is unnecessary and prone to be misused.

Why not put additional other parameters (batchOperation for now) in RequestContext?

@adoroszlai RequestContext is having wider information which is not required for any execution as can have misuse. Prototype and actual code will have some difference.
ExecutionContext:

index (ozone managed index)
ratisIndex (as present for old flow, may not be required once old flow is removed)

PostExecutionContext (new class for specific case)

ExecutionContext
batchOperation
...

Current way of passing all parameter via method signature is not a recommended practice, that is the reason we have findbug to restrict parameter count less than six. But we have misuse that by supressing. Recommended practice is to pass via pojo if number of paramenter increases.

Another mis-use done due to method restirction is use of Thread-Local.
Another mis-use, setting request object and other parameter in Base Class, and making complicated with derivation.

ExecutionContext (as pojo) is recommended way to pass parameter if parameter can increase, or have scope of expansion. I think with this, it can provide expansion of feature without impacting existing implementation.

So I think above explanation clarify its usecase and better way of coding (as suggested by findbug also).

We can not put all parameter now itself. Prototype implementation and actual can have little difference. Certain new area is discovered with respect to prototype, and are part of design doc.

devmadhuu

Refactoring changes LGTM +1. Follow up PR will bring Ozone Managed index changes as discussed.

adoroszlai

Thanks @sumitagrawl for the explanation. I'm still not convinced ExecutionContext is needed, but can accept it.

adoroszlai · 2025-01-04T08:03:00Z

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java

+    ExecutionContext context = new ExecutionContext();
+    context.setTermIndex(TransactionInfo.getTermIndex(transactionLogIndex));
+    context.setIndex(transactionLogIndex);


Please improve safety of ExecutionContext:

Remove mutators (set...)

Create private constructors

Make member variables final

Introduce factory methods like:

public static ExecutionContext of(long index) public static ExecutionContext of(TermIndex termIndex)

This reduces steps required in code that creates instances, and ensures only valid ExecutionContext can be created:

Suggested change

ExecutionContext context = new ExecutionContext();

context.setTermIndex(TransactionInfo.getTermIndex(transactionLogIndex));

context.setIndex(transactionLogIndex);

return validateAndUpdateCache(ozoneManager, ExecutionContext.of(transactionLogIndex));

If you plan to add many more members, then add builder instead of factory.

@adoroszlai updated ...

Thanks @sumitagrawl for updating the patch.

Original suggestion simplifies callers and also ensures index and termIndex.getIndex() are consistent:

public static ExecutionContext of(long index) public static ExecutionContext of(TermIndex termIndex)

@adoroszlai
This will be different in next PR, so created having together. Only this is to have most files impact to common place in this PR. So above can not be followed.

index: OM managed index -- it will be different of ratis index.
termIndex: Ratis index

But both is required to be part of ExecutionContext, used public static ExecutionContext of(long index, TermIndex termIndex)

Once number of params increases, will change to Builder pattern on need basis, which will have only few place to change.

Will you have non-Ratis index and Ratis TermIndex with different values at the same time?

yes,
ratis index is used in upgrade prepare and snapshot flow for specific purpose -- comparing if flush is completed or not.

Default, will be using OM managed index (non-ratis).

Once if removed ratis index in above cases with new flow, will remove this parameter in this class.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java

updated

kerneltime

This refactoring looks good, but the overall changes planned need more explanation. Since we are still using TermIndex and serializing it to ByteString and using it for DoubleBuffer, how does it change once we control the object ID generation and write batches to the ratis log.

kerneltime · 2025-01-07T17:30:43Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ozone.om.ratis;


Should be located in org.apache.hadoop.ozone.om instead? Since the goal is to make this independent of Ratis in the long run.

will follow below structure,

org.apache.hadoop.ozone.om.execution - factory/. // factory to create Execution handler - request/. // request handling - obs - OMKeyCreateExecutor.java - OMKeyCommitExecutor.java - fso - OMFileCreateExecutor.java .. - OMGateway.java - PreRatisExecutionEnabler.java - flowcontrol/ - ExecutionContext.java - PoolExecutor.java - PostExecutionContext.java - DbChangeRecorder.java - RequestContext.java - IndexGenerator.java. // generate unique index

kerneltime · 2025-01-07T17:31:19Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java

+import org.apache.ratis.server.protocol.TermIndex;
+
+/**
+ * context required for execution of request.


Suggested change

* context required for execution of request.

* Context required for execution of a request.

kerneltime · 2025-01-07T17:35:04Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java

+    if (null == termIndex) {
+      return TermIndex.valueOf(-1, index);
+    }


Move this to the constructor?

HDDS-11975. wrap TermIndex in ExecutionContext

c4a14b6

sumitagrawl requested a review from kerneltime December 20, 2024 12:32

sumitagrawl marked this pull request as ready for review December 20, 2024 13:55

adoroszlai reviewed Dec 22, 2024

View reviewed changes

devmadhuu reviewed Dec 23, 2024

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/ExecutionContext.java Outdated Show resolved Hide resolved

sumitagrawl requested review from devmadhuu and adoroszlai January 2, 2025 05:17

devmadhuu approved these changes Jan 3, 2025

View reviewed changes

adoroszlai previously requested changes Jan 4, 2025

View reviewed changes

fix review comment

2e53fe9

sumitagrawl requested a review from adoroszlai January 6, 2025 04:19

sumitagrawl added the om-pre-ratis-execution PRs related to https://issues.apache.org/jira/browse/HDDS-11897 label Jan 7, 2025

kerneltime approved these changes Jan 7, 2025

View reviewed changes

sumitagrawl added 2 commits January 8, 2025 10:33

fix review comment

86e3e00

add package-info

8405046

sumitagrawl merged commit f8394cf into apache:master Jan 8, 2025
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-11975. wrap TermIndex in ExecutionContext #7602

HDDS-11975. wrap TermIndex in ExecutionContext #7602

sumitagrawl commented Dec 20, 2024 •

edited

Loading

adoroszlai left a comment •

edited

Loading

sumitagrawl commented Dec 23, 2024

devmadhuu left a comment

adoroszlai commented Dec 23, 2024

sumitagrawl commented Dec 30, 2024 •

edited

Loading

devmadhuu left a comment

adoroszlai left a comment

adoroszlai Jan 4, 2025 •

edited

Loading

sumitagrawl Jan 6, 2025

adoroszlai Jan 6, 2025

sumitagrawl Jan 7, 2025 •

edited

Loading

adoroszlai Jan 7, 2025

sumitagrawl Jan 7, 2025 •

edited

Loading

kerneltime left a comment

kerneltime Jan 7, 2025

sumitagrawl Jan 8, 2025

kerneltime Jan 7, 2025

kerneltime Jan 7, 2025

	* context required for execution of request.
	* Context required for execution of a request.

HDDS-11975. wrap TermIndex in ExecutionContext #7602

HDDS-11975. wrap TermIndex in ExecutionContext #7602

Conversation

sumitagrawl commented Dec 20, 2024 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

adoroszlai left a comment • edited Loading

Choose a reason for hiding this comment

sumitagrawl commented Dec 23, 2024

devmadhuu left a comment

Choose a reason for hiding this comment

adoroszlai commented Dec 23, 2024

sumitagrawl commented Dec 30, 2024 • edited Loading

devmadhuu left a comment

Choose a reason for hiding this comment

adoroszlai left a comment

Choose a reason for hiding this comment

adoroszlai Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

sumitagrawl Jan 6, 2025

Choose a reason for hiding this comment

adoroszlai Jan 6, 2025

Choose a reason for hiding this comment

sumitagrawl Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

adoroszlai Jan 7, 2025

Choose a reason for hiding this comment

sumitagrawl Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

kerneltime left a comment

Choose a reason for hiding this comment

kerneltime Jan 7, 2025

Choose a reason for hiding this comment

sumitagrawl Jan 8, 2025

Choose a reason for hiding this comment

kerneltime Jan 7, 2025

Choose a reason for hiding this comment

kerneltime Jan 7, 2025

Choose a reason for hiding this comment

sumitagrawl commented Dec 20, 2024 •

edited

Loading

adoroszlai left a comment •

edited

Loading

sumitagrawl commented Dec 30, 2024 •

edited

Loading

adoroszlai Jan 4, 2025 •

edited

Loading

sumitagrawl Jan 7, 2025 •

edited

Loading

sumitagrawl Jan 7, 2025 •

edited

Loading