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.

Sign up for GitHub

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

Jump to bottom

Multisubmit rules #51

Merged
merged 9 commits into from
Jan 31, 2024
Merged

Multisubmit rules #51

merged 9 commits into from
Jan 31, 2024

Conversation

lumber1000
Copy link
Member

No description provided.

Nikita-Smirnov-Exactpro
Nikita-Smirnov-Exactpro requested changes Jan 18, 2024
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
src/main/proto/th2_grpc_check1/check1.proto Outdated
service Check1 {
rpc createCheckpoint (CheckpointRequest) returns (CheckpointResponse) {}
rpc submitCheckRule (CheckRuleRequest) returns (CheckRuleResponse) {}
rpc submitCheckSequenceRule(CheckSequenceRuleRequest) returns (CheckSequenceRuleResponse) {}
rpc submitNoMessageCheck(NoMessageCheckRequest) returns (NoMessageCheckResponse) {}
rpc waitForResult(WaitForResultRequest) returns (WaitForResultResponse) {}
rpc multiSubmitRules(MultiSubmitRulesRequest) returns (MultiSubmitRulesResponse) {}
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to submitMultipleRules(MultiRulesRequest) returns (MultiRulesResponse)

src/main/proto/th2_grpc_check1/check1.proto Outdated
Comment on lines 142 to 144
message MultiSubmitRulesResponse {
repeated RuleResponse responses = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message MultiSubmitRulesResponse {
repeated RuleResponse responses = 1;
}
message MultiRulesResponse {
ChainID chain_id = 1;
repeated RuleResponse responses = 2;
}

src/main/proto/th2_grpc_check1/check1.proto Show resolved Hide resolved
@Nikita-Smirnov-Exactpro
Copy link
Member

I have questions about chain id in submit multiple rules.

  1. Should check1 provide ability to submit rules to different chain id? It is related to chain id in each request
  2. Should check1 create separate chain id for each rules? It is related to chain id in response

My opinion is check1 should handle the whole multiple rules request as request for one chain id. This behavior should cover test case needs, because as I know users doesn't use chain id or use fixed chain id for case.
@OptimumCode what do you think about these questions?

@lumber1000
Copy link
Member Author

Currently check1 allows to submit to one chain or to multiple chains or to generate new chainId for every rule. Demo script uses both fixed chainId for some rules and generating new chainId in check1 for others.

@OptimumCode
Copy link
Member

My opinion is check1 should handle the whole multiple rules request as request for one chain id. This behavior should cover test case needs, because as I know users doesn't use chain id or use fixed chain id for case. @OptimumCode what do you think about these questions?

I don't think it will look good with the current API state. Each rule request contains ChainID. So, it is expected that this chain ID will be used when the rule is submitted. At least I would expect that. If we apply your suggestion @Nikita-Smirnov-Exactpro the above won't be true and will look like an unexpected behavior for the user in my opinion.
We could introduce a top-level chain ID in MultiRulesRequest that could be used for all rules if a user didn't specify a chain ID for a particular rule. The same might be applied to the MultiRulesResponse, however, we should not remove the chain ID from the individual response per rule as some rules might have their own chain IDs.

With the current API, I would rather stick to the option the @lumber1000 used in the beginning.

@Nikita-Smirnov-Exactpro
Copy link
Member

We could introduce a top-level chain ID in MultiRulesRequest that could be used for all rules if a user didn't specify a chain ID for a particular rule. The same might be applied to the MultiRulesResponse, however, we should not remove the chain ID from the individual response per rule as some rules might have their own chain IDs.

I like this option, in this case user can submit multiple rules under single chain.
Also, I would like to use common chain id as priority way, for example

top-level-chain-id: empty
  rule1-chain-d: empty
  rule2-chain-d: chain-a
  rule3-chain-d: empty

check1 generates new chain-0 chain id and submits 1,3 rule under it. Responses for each rule has actual chain id

'default_chain' added to 'MultiRulesRequest'
85adf67 
 'postMultipleRules' method added
@lumber1000 lumber1000 marked this pull request as ready for review January 30, 2024 16:52
Merge branch 'dev-version-4' into multisubmit-dv4
9313a58 
# Conflicts:
#	README.md
#	build.gradle
#	gradle.properties
#	package_info.json
@lumber1000 lumber1000 merged commit aa4b12c into dev-version-4 Jan 31, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro approved these changes

@OptimumCode OptimumCode Awaiting requested review from OptimumCode

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lumber1000 @Nikita-Smirnov-Exactpro @OptimumCode