Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5854

Introduce some Flink-specific base Exception types

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Core
    • Labels:
      None

      Description

      Going through the code, there are a lot of places where exception handling could be done a bit nicer, for example

      • Some methods do not declare exceptions at all in their signatures. They simply catch all and wrap it in a RuntimeException.
      • Some places declare overly generic that they throw Exception, even though they could very specifically type the exceptions they throw.

      I suggest to introduce two new basic exceptions, that at least help document a bit more what goes wrong:

      • FlinkException as a base class for checked exceptions that indicate that something related to using Flink went wrong. Letting a method throw FlinkException rather than Exception already helps to not include all of Java's runtime exceptions, which indicate programming errors, rather than situations that should be recovered.
      • FlinkUncheckedException as a Flink-specific subclass of RuntimeException. That one can come in handy in places where no exceptions were declared, for example when reusing an interface that does not declare exceptions.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user StephanEwen opened a pull request:

          https://github.com/apache/flink/pull/3368

          FLINK-5854 [core] Add base Flink Exception classes

          This pull request adds two exception base classes: `FlinkException` and `FlinkRuntimeException`.
          They are useful in improving the way certain parts of the code handle exceptions.

          • `FlinkException` is a base class for checked exceptions that indicate that something related to using Flink went wrong. It is helpful, because letting a method throw `FlinkException` rather than `Exception` already helps to not include all of Java's runtime exceptions, which indicate programming errors, rather than situations that should be recovered.
          • `FlinkRuntimeException` as a Flink-specific subclass of `RuntimeException` comes in handy in places where no exceptions were declared, for example when reusing an interface that does not declare exceptions.

          *Important: This does not mean we should just declare `FlinkException` everywhere and throw and catch `FlinkException` and `FlinkRuntimeException` arbitrarily. Exception handling remains a careful and conscious task.*

          This also adds the `DynamicCodeLoadingException` subclass of `FlinkException` as an example.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/StephanEwen/incubator-flink exceptions

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3368.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3368


          commit 1bed2d20a5ccfae4ae7bdfadaaf03fcbe1dba449
          Author: Stephan Ewen <sewen@apache.org>
          Date: 2017-02-17T15:24:35Z

          [FLINK-XXXX] [core] Add base Flink Exception classes


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user StephanEwen opened a pull request: https://github.com/apache/flink/pull/3368 FLINK-5854 [core] Add base Flink Exception classes This pull request adds two exception base classes: `FlinkException` and `FlinkRuntimeException`. They are useful in improving the way certain parts of the code handle exceptions. `FlinkException` is a base class for checked exceptions that indicate that something related to using Flink went wrong. It is helpful, because letting a method throw `FlinkException` rather than `Exception` already helps to not include all of Java's runtime exceptions, which indicate programming errors, rather than situations that should be recovered. `FlinkRuntimeException` as a Flink-specific subclass of `RuntimeException` comes in handy in places where no exceptions were declared, for example when reusing an interface that does not declare exceptions. * Important: This does not mean we should just declare `FlinkException` everywhere and throw and catch `FlinkException` and `FlinkRuntimeException` arbitrarily. Exception handling remains a careful and conscious task. * This also adds the `DynamicCodeLoadingException` subclass of `FlinkException` as an example. You can merge this pull request into a Git repository by running: $ git pull https://github.com/StephanEwen/incubator-flink exceptions Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3368.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3368 commit 1bed2d20a5ccfae4ae7bdfadaaf03fcbe1dba449 Author: Stephan Ewen <sewen@apache.org> Date: 2017-02-17T15:24:35Z [FLINK-XXXX] [core] Add base Flink Exception classes
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3368#discussion_r102071979

          — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java —
          @@ -0,0 +1,52 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.util;
          +
          +import org.apache.flink.annotation.Public;
          +
          +/**
          + * An exception that is thrown if the dynamic instantiation of code fails.
          + *
          + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around
          + * dynamic code loading and instantiations:
          + *
          + * <pre>{@code
          + * try

          { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * }

          + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) {
          + * throw new DynamicCodeLoadingException(e);
          — End diff –

          there is no constructor that matches this line of the javadoc.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3368#discussion_r102071979 — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java — @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.util; + +import org.apache.flink.annotation.Public; + +/** + * An exception that is thrown if the dynamic instantiation of code fails. + * + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around + * dynamic code loading and instantiations: + * + * <pre>{@code + * try { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * } + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) { + * throw new DynamicCodeLoadingException(e); — End diff – there is no constructor that matches this line of the javadoc.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3368#discussion_r102072425

          — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java —
          @@ -0,0 +1,52 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.util;
          +
          +import org.apache.flink.annotation.Public;
          +
          +/**
          + * An exception that is thrown if the dynamic instantiation of code fails.
          + *
          + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around
          + * dynamic code loading and instantiations:
          + *
          + * <pre>{@code
          + * try

          { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * }

          + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e)

          { + * throw new DynamicCodeLoadingException(e); + * }

          + * }</pre>
          + */
          +@Public
          +public class DynamicCodeLoadingException extends FlinkException {
          +
          + private static final long serialVersionUID = -25138443817255490L;
          +
          + /**
          + * Creates a new exception with the given message and cause
          + *
          + * @param message The exception message
          + * @param cause The exception that caused this exception
          + */
          + public DynamicCodeLoadingException(String message, Throwable cause) {
          — End diff –

          Would it make sense to make this constructor more explicit in the type of exceptions it accepts? (i.e. one constructor each for the exceptions that are typically thrown in situations that we want to cover)

          It's probably just be bloat, but maybe it would prevent misuse of this exception.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3368#discussion_r102072425 — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java — @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.util; + +import org.apache.flink.annotation.Public; + +/** + * An exception that is thrown if the dynamic instantiation of code fails. + * + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around + * dynamic code loading and instantiations: + * + * <pre>{@code + * try { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * } + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) { + * throw new DynamicCodeLoadingException(e); + * } + * }</pre> + */ +@Public +public class DynamicCodeLoadingException extends FlinkException { + + private static final long serialVersionUID = -25138443817255490L; + + /** + * Creates a new exception with the given message and cause + * + * @param message The exception message + * @param cause The exception that caused this exception + */ + public DynamicCodeLoadingException(String message, Throwable cause) { — End diff – Would it make sense to make this constructor more explicit in the type of exceptions it accepts? (i.e. one constructor each for the exceptions that are typically thrown in situations that we want to cover) It's probably just be bloat, but maybe it would prevent misuse of this exception.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3368#discussion_r102072659

          — Diff: flink-core/src/main/java/org/apache/flink/util/FlinkRuntimeException.java —
          @@ -0,0 +1,65 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.util;
          +
          +import org.apache.flink.annotation.Public;
          +
          +/**
          + * Base class of all Flink-specific unchecked exceptions.
          + */
          +@Public
          +public class FlinkRuntimeException extends RuntimeException {
          +
          + private static final long serialVersionUID = 193141189399279147L;
          +
          + /**
          + * Creates a new exception with a null message and null cause.
          + */
          + public FlinkRuntimeException() {
          — End diff –

          Is there a reasonable use-case for an exception without an error message or cause?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3368#discussion_r102072659 — Diff: flink-core/src/main/java/org/apache/flink/util/FlinkRuntimeException.java — @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.util; + +import org.apache.flink.annotation.Public; + +/** + * Base class of all Flink-specific unchecked exceptions. + */ +@Public +public class FlinkRuntimeException extends RuntimeException { + + private static final long serialVersionUID = 193141189399279147L; + + /** + * Creates a new exception with a null message and null cause. + */ + public FlinkRuntimeException() { — End diff – Is there a reasonable use-case for an exception without an error message or cause?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3368

          Could you name 1 or 2 examples for situations where you think it is appropriate to throw a ```FlinkException```? Would invalid arguments (like a String being null) be a reason to do so?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3368 Could you name 1 or 2 examples for situations where you think it is appropriate to throw a ```FlinkException```? Would invalid arguments (like a String being null) be a reason to do so?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3368

          @zentol There are many places in the runtime that declare `throws Exception`, for example virtually all of the state handling code. This always came from the desire to throw `IOException` plus something that expresses that non-I/O stuff related to Flink went wrong. The result was a `throws Exception`, which also means that you have to catch `Exception` which you often don't want (because this included `RuntimeException` and you typically want runtime exception to bubble up a bit further, since they denote bugs by encouraged design).

          The only place where `throws Exception` really makes sense to me is for `MapFunction` and the likes, to allow them to propagate any type of exception and let recovery handle them.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3368 @zentol There are many places in the runtime that declare `throws Exception`, for example virtually all of the state handling code. This always came from the desire to throw `IOException` plus something that expresses that non-I/O stuff related to Flink went wrong. The result was a `throws Exception`, which also means that you have to catch `Exception` which you often don't want (because this included `RuntimeException` and you typically want runtime exception to bubble up a bit further, since they denote bugs by encouraged design). The only place where `throws Exception` really makes sense to me is for `MapFunction` and the likes, to allow them to propagate any type of exception and let recovery handle them.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3368

          It is arguable whether exceptions should ever have a constructor without a message, I simply did that here for convenience. I have no strong feelings about removing the zero argument constructors.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3368 It is arguable whether exceptions should ever have a constructor without a message, I simply did that here for convenience. I have no strong feelings about removing the zero argument constructors.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3368#discussion_r102430926

          — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java —
          @@ -0,0 +1,52 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.util;
          +
          +import org.apache.flink.annotation.Public;
          +
          +/**
          + * An exception that is thrown if the dynamic instantiation of code fails.
          + *
          + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around
          + * dynamic code loading and instantiations:
          + *
          + * <pre>{@code
          + * try

          { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * }

          + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) {
          + * throw new DynamicCodeLoadingException(e);
          — End diff –

          Agreed, will fix this

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3368#discussion_r102430926 — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java — @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.util; + +import org.apache.flink.annotation.Public; + +/** + * An exception that is thrown if the dynamic instantiation of code fails. + * + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around + * dynamic code loading and instantiations: + * + * <pre>{@code + * try { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * } + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) { + * throw new DynamicCodeLoadingException(e); — End diff – Agreed, will fix this
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3368#discussion_r102431100

          — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java —
          @@ -0,0 +1,52 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.util;
          +
          +import org.apache.flink.annotation.Public;
          +
          +/**
          + * An exception that is thrown if the dynamic instantiation of code fails.
          + *
          + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around
          + * dynamic code loading and instantiations:
          + *
          + * <pre>{@code
          + * try

          { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * }

          + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e)

          { + * throw new DynamicCodeLoadingException(e); + * }

          + * }</pre>
          + */
          +@Public
          +public class DynamicCodeLoadingException extends FlinkException {
          +
          + private static final long serialVersionUID = -25138443817255490L;
          +
          + /**
          + * Creates a new exception with the given message and cause
          + *
          + * @param message The exception message
          + * @param cause The exception that caused this exception
          + */
          + public DynamicCodeLoadingException(String message, Throwable cause) {
          — End diff –

          There may be other cases for this exception that we do not anticipate, like for shipping of generated code in bytes that is dynamically converted to classes (not sure what exceptions that will entail, but probably `ClassFormatError` and so on).

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3368#discussion_r102431100 — Diff: flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java — @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.util; + +import org.apache.flink.annotation.Public; + +/** + * An exception that is thrown if the dynamic instantiation of code fails. + * + * <p>This exception is supposed to "sum up" the zoo of exceptions typically thrown around + * dynamic code loading and instantiations: + * + * <pre>{@code + * try { + * Class.forName(classname).asSubclass(TheType.class).newInstance(); + * } + * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) { + * throw new DynamicCodeLoadingException(e); + * } + * }</pre> + */ +@Public +public class DynamicCodeLoadingException extends FlinkException { + + private static final long serialVersionUID = -25138443817255490L; + + /** + * Creates a new exception with the given message and cause + * + * @param message The exception message + * @param cause The exception that caused this exception + */ + public DynamicCodeLoadingException(String message, Throwable cause) { — End diff – There may be other cases for this exception that we do not anticipate, like for shipping of generated code in bytes that is dynamically converted to classes (not sure what exceptions that will entail, but probably `ClassFormatError` and so on).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3368

          Any reservations against merging this after addressing the comments above?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3368 Any reservations against merging this after addressing the comments above?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3368

          This looks good, but what is the reason behind the empty constructors that create an exception without message or cause?

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3368 This looks good, but what is the reason behind the empty constructors that create an exception without message or cause?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uce commented on the issue:

          https://github.com/apache/flink/pull/3368

          Hey Stephan! The changes look very good. Thanks also for your explanations, I think this is something that we should focus on more during code reviews. Actually, a section in the Internals or Contribution docs or Wiki would be helpful.

          A question by example:
          Curator `throws Exception` on most operations. Previously, this was simply forwarded. Should this now be wrapped in a `FlinkRuntimeException`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on the issue: https://github.com/apache/flink/pull/3368 Hey Stephan! The changes look very good. Thanks also for your explanations, I think this is something that we should focus on more during code reviews. Actually, a section in the Internals or Contribution docs or Wiki would be helpful. A question by example: Curator `throws Exception` on most operations. Previously, this was simply forwarded. Should this now be wrapped in a `FlinkRuntimeException`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3368

          Big +1 from my side. I think that more specific exceptions helps to make people think more about the cause of an exception and, thus, also how it should be handled. Especially the fact that we always catch `RuntimeException` when having `throws Exception` in the method definition annoyed me a lot.

          @uce I think we should not per se wrap all current `Exceptions` into a `FlinkRuntimeException`. Better to do a case distinction here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3368 Big +1 from my side. I think that more specific exceptions helps to make people think more about the cause of an exception and, thus, also how it should be handled. Especially the fact that we always catch `RuntimeException` when having `throws Exception` in the method definition annoyed me a lot. @uce I think we should not per se wrap all current `Exceptions` into a `FlinkRuntimeException`. Better to do a case distinction here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3368

          Thanks for the comments. Will address the issues and remove the "no message, no cause" constructors. We should not encourage exceptions without information.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3368 Thanks for the comments. Will address the issues and remove the "no message, no cause" constructors. We should not encourage exceptions without information.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3368

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3368
          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed via 813c2585a49c673b71678463d719b6a85b778994

          Show
          StephanEwen Stephan Ewen added a comment - Fixed via 813c2585a49c673b71678463d719b6a85b778994

            People

            • Assignee:
              StephanEwen Stephan Ewen
              Reporter:
              StephanEwen Stephan Ewen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development