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.

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

Add support for changing JobConfig of a suspended job [HZ-1650] #23862

Merged
merged 27 commits into from
Mar 9, 2023

Conversation

viliam-durina
Copy link
Contributor

@viliam-durina viliam-durina commented Mar 7, 2023

Replaces #23523.
Adds support for changing JobConfig of a suspended job.
Protocol changes: hazelcast/hazelcast-client-protocol#450

@viliam-durina viliam-durina added Type: Enhancement Team: SQL Add to Release Notes SQL-only Use when changes are only in hazelcast-sql module labels Mar 7, 2023
@viliam-durina viliam-durina added this to the 5.3.0 milestone Mar 7, 2023
@viliam-durina viliam-durina marked this pull request as ready for review March 7, 2023 10:16
@viliam-durina viliam-durina requested a review from a team as a code owner March 7, 2023 10:16
@viliam-durina viliam-durina removed SQL-only Use when changes are only in hazelcast-sql module Add to Release Notes labels Mar 7, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 7, 2023
@k-jamroz
Copy link
Contributor

k-jamroz commented Mar 7, 2023

we should add tests in EE that ensure that config can be changed in SUSPENDED_EXPORTING_SNAPSHOT state

Copy link
Contributor

@k-jamroz k-jamroz left a comment

Choose a reason for hiding this comment

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

need to restore check in PlanExecutor.execute(CreateJobPlan plan, List<Object> arguments) - for non streaming jobs (maybe with comment?) and it will be ok

Copy link
Contributor

@mdumandag mdumandag left a comment

Choose a reason for hiding this comment

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

LGTM overall from the client's PoV, I have some minor comments here as well as in the protocol PR

The test relied that INSERT doesn't insert any rows, if it fails with
`AccumulationLimitExceededException`. But it might have inserted some rows, and
if it manages to insert them, the query will fail with "Duplicate key" after
restart.

The new test uses a window aggregation with three keys. This fails if max
accumulated records is 2, before emitting anything from the aggregation, so the
INSERT doesn't fail.
The test relied on window aggregation, but window aggregation didn't support the
max accumulation limit. It actually failed due to the insert failing, which was
non-deterministic for the same reason as the previous fix.

This PR replaces INSERT with SINK, uses more keys (because they are spread
across multiple processors), and also adds the accumulation check to the
SlidingWindowP.
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 8, 2023
@k-jamroz k-jamroz mentioned this pull request Mar 8, 2023
5 tasks
# Conflicts:
#	hazelcast/src/main/java/com/hazelcast/jet/impl/ClientJobProxy.java
#	hazelcast/src/main/java/com/hazelcast/jet/impl/client/protocol/task/JetMessageTaskFactoryProvider.java
#	hazelcast/src/test/java/com/hazelcast/client/protocol/compatibility/ClientCompatibilityNullTest_2_6.java
#	hazelcast/src/test/java/com/hazelcast/client/protocol/compatibility/ClientCompatibilityTest_2_6.java
#	hazelcast/src/test/java/com/hazelcast/client/protocol/compatibility/MemberCompatibilityNullTest_2_6.java
#	hazelcast/src/test/java/com/hazelcast/client/protocol/compatibility/MemberCompatibilityTest_2_6.java
#	hazelcast/src/test/resources/2.6.protocol.compatibility.binary
#	hazelcast/src/test/resources/2.6.protocol.compatibility.null.binary
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
# Conflicts:
#	hazelcast/src/test/resources/2.6.protocol.compatibility.binary
#	hazelcast/src/test/resources/2.6.protocol.compatibility.null.binary
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
@hazelcast hazelcast deleted a comment from hz-devops-test Mar 9, 2023
@viliam-durina viliam-durina merged commit 68dd004 into hazelcast:master Mar 9, 2023
@viliam-durina viliam-durina deleted the feature/set-job-config branch March 9, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants