Skip to content

Conversation

@rodmeneses
Copy link
Contributor

What is the purpose of the change

The operator often hides the actual issue for deployment errors and do not correctly report the error events.

Brief change log

We are reporting the first 3 possible exceptions in the exception hierarchy in the error events.

Verifying this change

This change added tests and can be verified as follows:

  • Added new unit tests

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: no

Documentation

  • Does this pull request introduce a new feature? no

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

/** Test for {@link ExceptionUtils}. */
public class ExceptionUtilsTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

So many test cases feel a bit over the top, could we narrow it down to 2-3 cases that cover the functionality?

var ex = new RuntimeException("Cause 1", new SerializedThrowable(ex2));
assertThat(ExceptionUtils.getExceptionMessage(ex))
.isEqualTo("Cause 1 -> Cause 2 -> Cause 3");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a test for the non serializable case such as:

    @Test
    void testSerializedThrowableError() {
        var serializedException = new SerializedThrowable(new NonSerializableException());
        assertThat(ExceptionUtils.getExceptionMessage(serializedException))
                .isEqualTo("Unknown Error (SerializedThrowable)");
    }

    private static class NonSerializableException extends Exception {
        private void writeObject(java.io.ObjectOutputStream stream) throws IOException {
            throw new IOException();
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!

@rodmeneses rodmeneses requested a review from gyfora March 25, 2025 22:27
@gyfora
Copy link
Contributor

gyfora commented Mar 27, 2025

The e2es for Flink 2.0 started to fail after the official release, so we have new docker images. Merging this to fix the maven build then going to work on fixing the 2.0 compatibility.

@gyfora gyfora merged commit 7a65e02 into apache:main Mar 27, 2025
92 of 121 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants