Skip to content

fix: multiple dependents of same type exceptions #2226

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

Merged
merged 3 commits into from
Feb 6, 2024
Merged

fix: multiple dependents of same type exceptions #2226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4ffd6c
fix: multiple dependents of same type exceptions
csviri Feb 1, 2024
8019393
extract delimiter
csviri Feb 1, 2024
93b611d
fix: do not expose underlying de-duplication mechanism
metacosm Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions ...c/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
package io.javaoperatorsdk.operator.processing.dependent.workflow;

import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;
import java.util.stream.Collectors;

import io.javaoperatorsdk.operator.AggregatedOperatorException;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;

@SuppressWarnings("rawtypes")
class WorkflowResult {

private static final String NUMBER_DELIMITER = "_";
private final Map<DependentResource, Exception> erroredDependents;

WorkflowResult(Map<DependentResource, Exception> erroredDependents) {
Expand All @@ -36,9 +37,22 @@ public boolean erroredDependentsExist() {

public void throwAggregateExceptionIfErrorsPresent() {
if (erroredDependentsExist()) {
Map<String, Exception> exceptionMap = new HashMap<>();
Map<String, Integer> numberOfClasses = new HashMap<>();

for (Entry<DependentResource, Exception> entry : erroredDependents.entrySet()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that numbering the dependents help here since people might give meaning to the number and/or wonder where the numbers come from. Also, since we're processing an unordered collection, the numbers might "identify" different dependents over time making things confusing, imo. I think it would be better to find a stable way to discriminate between DependentResources of the same type and use that rather than an arbitrary number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sure, so this is basically just a temp, solution created issue for a proper one:
#2227

Note that this is quite rare that something like this would happen in normal circumstances, having the same type of DR in a workflow and at least 2 having exception in same time.

(Maybe a randomized string would be also good there?)

String name = entry.getKey().getClass().getName();
var num = numberOfClasses.getOrDefault(name, 0);
if (num > 0) {
exceptionMap.put(name + NUMBER_DELIMITER + num, entry.getValue());
} else {
exceptionMap.put(name, entry.getValue());
}
numberOfClasses.put(name, num + 1);
}

throw new AggregatedOperatorException("Exception(s) during workflow execution.",
erroredDependents.entrySet().stream()
.collect(Collectors.toMap(e -> e.getKey().getClass().getName(), Entry::getValue)));
exceptionMap);
}
}
}
65 changes: 65 additions & 0 deletions ...st/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package io.javaoperatorsdk.operator.processing.dependent.workflow;

import java.util.Map;

import org.junit.jupiter.api.Test;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.AggregatedOperatorException;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;

import static org.assertj.core.api.Assertions.assertThat;

class WorkflowResultTest {

@Test
void throwsExceptionWithoutNumberingIfAllDifferentClass() {
var res = new WorkflowResult(Map.of(new DependentA(), new RuntimeException(),
new DependentB(), new RuntimeException()));
try {
res.throwAggregateExceptionIfErrorsPresent();
} catch (AggregatedOperatorException e) {
assertThat(e.getAggregatedExceptions()).containsOnlyKeys(DependentA.class.getName(),
DependentB.class.getName());
}
}

@Test
void numbersDependentClassNamesIfMoreOfSameType() {
var res = new WorkflowResult(Map.of(new DependentA(), new RuntimeException(),
new DependentA(), new RuntimeException()));
try {
res.throwAggregateExceptionIfErrorsPresent();
} catch (AggregatedOperatorException e) {
assertThat(e.getAggregatedExceptions()).hasSize(2);
}
}

@SuppressWarnings("rawtypes")
static class DependentA implements DependentResource {
@Override
public ReconcileResult reconcile(HasMetadata primary, Context context) {
return null;
}

@Override
public Class resourceType() {
return null;
}
}

@SuppressWarnings("rawtypes")
static class DependentB implements DependentResource {
@Override
public ReconcileResult reconcile(HasMetadata primary, Context context) {
return null;
}

@Override
public Class resourceType() {
return null;
}
}
}