Allow configuring additional options when restarting connectors#11797
Allow configuring additional options when restarting connectors#11797ppatierno merged 29 commits intostrimzi:mainfrom
Conversation
|
@rlanhellas there are some checkstyle failures within the Azure pipeline. |
ppatierno
left a comment
There was a problem hiding this comment.
Thanks @rlanhellas! I was wondering we need some tests about the new annotation values to check that also patterns are valid.
| if (!restartAnnotationIsValid(resource)) { | ||
| String message = "Invalid annotation format"; | ||
| LOGGER.warnCr(reconciliation, message); | ||
| conditions.add(StatusUtils.buildWarningCondition("RestartConnector", message)); |
There was a problem hiding this comment.
I think we can just put the string in the log and condition even if it's the same. This is anyway what the compiler is going to do. Not sure how much value the message variable can bring here, considering that the two lines using the message are close each other and not spread across the codebase.
| /** | ||
| * Pattern for validation of MirrorMaker2 restart connector annotation value. | ||
| * */ | ||
| public static final Pattern ANNO_STRIMZI_IO_RESTART_CONNECTOR_ARGS_PATTERN = Pattern |
There was a problem hiding this comment.
if it's just for MirrorMaker 2 maybe we should state it in the name (even just adding MM2 somewhere would be ok for me).
There was a problem hiding this comment.
done, just added MM2
agree, I'm writing tests to this, will update this PR soon. |
fixed |
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some nits. You should also:
- fix the title of the PR
- Add it to the CHANGELOG file
- Add it to the docs
| /** | ||
| * Pattern for validation of restart connector annotation value. | ||
| * */ | ||
| public static final Pattern ANNO_STRIMZI_IO_RESTART_ARGS_PATTERN = Pattern.compile("^includeTasks," + | ||
| "onlyFailed$|^onlyFailed,includeTasks$|^includeTasks$|^onlyFailed$|^true$"); | ||
|
|
||
| /** | ||
| * Pattern for validation of MirrorMaker2 restart connector annotation value. | ||
| * */ | ||
| public static final Pattern ANNO_STRIMZI_IO_RESTART_CONNECTOR_MM2_ARGS_PATTERN = Pattern | ||
| .compile("^([a-zA-Z0-9-_]+):includeTasks,onlyFailed$|^([a-zA-Z0-9-_]+):onlyFailed,includeTasks$" + | ||
| "|^([a-zA-Z0-9-_]+):includeTasks$|^([a-zA-Z0-9-_]+):onlyFailed$|^([a-zA-Z0-9-_]+)$"); | ||
|
|
||
| /** | ||
| * This optional argument can be used to include tasks in the restart connector operation. | ||
| * */ | ||
| public static final String ANNO_STRIMZI_IO_RESTART_INCLUDE_TASKS_ARG = "includeTasks"; | ||
|
|
||
| /** | ||
| * This optional argument can be used to restart connector only failed tasks. | ||
| * */ | ||
| public static final String ANNO_STRIMZI_IO_RESTART_ONLY_FAILED_ARG = "onlyFailed"; | ||
|
|
There was a problem hiding this comment.
As these are not really annotations, can we keep these internally in the corresponding classes?
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean restartAnnotationHasOnlyFailedTasksArg(CustomResource resource) { |
There was a problem hiding this comment.
These methods can probably use the HasMetadata interface instead of CustomResource? The same applies also to the Connect methods.
| if (!restartAnnotationIsValid(resource)) { | ||
| LOGGER.warnCr(reconciliation, "Invalid annotation format"); | ||
| conditions.add(StatusUtils.buildWarningCondition("RestartConnector", "Invalid annotation format")); | ||
| return Future.succeededFuture(conditions); | ||
| } | ||
| boolean restartIncludeTasks = restartAnnotationHasIncludeTasksArg(resource); | ||
| boolean restartOnlyFailedTasks = restartAnnotationHasOnlyFailedTasksArg(resource); | ||
| LOGGER.infoCr(reconciliation, "Restarting connector {}", connectorName); | ||
| return VertxUtil.completableFutureToVertxFuture(apiClient.restart(host, port, connectorName, restartIncludeTasks, restartOnlyFailedTasks)) |
There was a problem hiding this comment.
You should probably add some spacing (empty lines) for better readability.
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
d212b7d to
85c3896
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11797 +/- ##
============================================
+ Coverage 67.51% 67.55% +0.03%
- Complexity 7080 7101 +21
============================================
Files 574 574
Lines 28134 28181 +47
Branches 3187 3201 +14
============================================
+ Hits 18995 19037 +42
+ Misses 7824 7820 -4
- Partials 1315 1324 +9
🚀 New features to boost your workflow:
|
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
|
everything is done @scholzj, @ppatierno , if you can take a look again |
| private static final Pattern STRIMZI_IO_RESTART_ARGS_PATTERN = Pattern.compile("^includeTasks," + | ||
| "onlyFailed$|^onlyFailed,includeTasks$|^includeTasks$|^onlyFailed$|^true$"); |
There was a problem hiding this comment.
This seems pretty hacky. Maybe you can split it by , into a list and check the values or something?
There was a problem hiding this comment.
I changed the validation to if-else statement and removed the regex pattern
| abstract boolean restartAnnotationIsValid(CustomResource resource); | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument includeTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument includeTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationHasIncludeTasksArg(CustomResource resource); | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument onlyFailedTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument onlyFailedTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationHasOnlyFailedTasksArg(HasMetadata resource); |
There was a problem hiding this comment.
Did you check here and with the implementations if you can use HasMetadata instead of CustomResource?
There was a problem hiding this comment.
I forgot to change the implementations, just did it
| private static final Pattern STRIMZI_IO_RESTART_CONNECTOR_MM2_ARGS_PATTERN = Pattern | ||
| .compile("^([a-zA-Z0-9-_]+):includeTasks,onlyFailed$|^([a-zA-Z0-9-_]+):onlyFailed,includeTasks$" + | ||
| "|^([a-zA-Z0-9-_]+):includeTasks$|^([a-zA-Z0-9-_]+):onlyFailed$|^([a-zA-Z0-9-_]+)$"); |
There was a problem hiding this comment.
Same as in the other expression. Maybe we can try to simplify it a bit? Parse our the comnand part using the Regexp and than split it rather than including all various combiantions? This does not scale very well.
In addition, whatever the way to parse it is, it might be good to have some unit tests for how it parses the various annotations (both here as well as for connect). With some valid and invalid values etc.
There was a problem hiding this comment.
I changed the validation to if-else statement and removed the regex pattern
There was a problem hiding this comment.
Btw, regarding testing invalid args, you can check the tests, I added a test to invalid args
There was a problem hiding this comment.
I see. It is probably good to test it with invalid value once in the way you have it.
But why don't you just test the methods for validating the annotations / checking the presence of the arguments? You can easily test them with 10 different valid and invalid values without the need for any mocks or anything. That lets you very quickly test all the possibly annotation forms with typos, spaces, etc.
There was a problem hiding this comment.
I created a unit test for both kafka connect and MM2 annotation values with many cases to test the maximum of validations we can. Please take a look again.
Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
…rator/assembly/KafkaMirrorMaker2AssemblyOperatorConnectorRestartTest.java Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
| * @return True if the provided resource has valid restart annotation. False otherwise. | ||
| * */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationIsValid(CustomResource resource, String connectorName); |
There was a problem hiding this comment.
HasMetadata here and its implementations as well?
Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
…ka connector and mm2 Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
…ka connector and mm2 Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
scholzj
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the changes. I left few more comments. But it looks mostly good.
There was a problem hiding this comment.
Hmm, I did not realize how complicated this would be. I wonder if we should make the methods static. But maybe we should leave that for some future refactoring.
In any case, I do not think there is any reason to have this in a separate file. I don't think we need so many separate test classes just for a single feature like this. Just move it to cluster-operator/src/test/java/io/strimzi/operator/cluster/operator/assembly/KafkaConnectAssemblyOperatorConnectorRestartTest.java maybe?
There was a problem hiding this comment.
Same as with the other file. I would move it to cluster-operator/src/test/java/io/strimzi/operator/cluster/operator/assembly/KafkaMirrorMaker2AssemblyOperatorConnectorRestartTest.java.
scholzj
left a comment
There was a problem hiding this comment.
Two more nits. LGTM otherwise. Thanks for the work an patience.
Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
…rator/assembly/AbstractConnectOperator.java Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: Paolo Patierno <paolo.patierno@gmail.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: Paolo Patierno <paolo.patierno@gmail.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: Paolo Patierno <paolo.patierno@gmail.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
PaulRMellor
left a comment
There was a problem hiding this comment.
I made a suggestion on the docs to include a bit more detail on the options and to format with the step
Co-authored-by: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
katheris
left a comment
There was a problem hiding this comment.
Thanks for working on this PR @rlanhellas. The code look great, I just had a couple of test related questions which I added
|
|
||
| op.maybeCreateOrUpdateConnector(Reconciliation.DUMMY_RECONCILIATION, "my-connect-host", mockConnectApi, "my-connector", connector.getSpec(), kafkaMirrorMaker2) | ||
| .onComplete(context.succeeding(r -> context.verify(() -> { | ||
| verify(mockConnectApi, never()).restart(any(), anyInt(), any(), eq(false), eq(false)); |
There was a problem hiding this comment.
Same here as the Connect tests, I think these arguments matches shouldn't check for eq(false)
PaulRMellor
left a comment
There was a problem hiding this comment.
Thanks for the updates
|
@rlanhellas do you have any ETA for the changes requested by Kate? The plan was to cut the 0.48.0 branch today and start the release process for RC1 and we would like to have this one included as planned. Thanks! |
|
My plan is to have it ready tomorrow morning, may we wait until tomorrow to cut the branch ? @ppatierno |
@rlanhellas yeah sure no worries. I have got a "little" thing I would like to include in 0.48.0 so there is still time for it. But if you can do by tomorrow would be great, so when I am ready, I can just proceed with the release because yours work is already in. |
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
…rator/assembly/KafkaConnectAssemblyOperatorConnectorRestartTest.java Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
Signed-off-by: Ronaldo Lanhellas <ronaldo.lanhellas@gmail.com>
katheris
left a comment
There was a problem hiding this comment.
Thanks for all your work on this @rlanhellas
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Type of change
Enhancement
Description
Fixes #11762 #11484
This PR implements this proposal: https://github.com/strimzi/proposals/blob/main/111-add-restart-parameters-to-kafka-connectors.md.
Checklist