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

queryable state: KvStateRequestSerializer#deserializeKeyAndNamespace() throws an IOException without own failure message if deserialisation fails

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.2.0, 1.3.0
    • Component/s: Queryable State
    • Labels:
      None

      Description

      KvStateRequestSerializer#deserializeKeyAndNamespace() throws an IOException, e.g. EOFException, if the deserialisation fails, e.g. there are not enough available bytes.
      In these cases, it should instead also throw an IllegalArgumentException with a message containing "This indicates a mismatch in the key/namespace serializers used by the KvState instance and this access." as the other error cases.

        Issue Links

          Activity

          Hide
          NicoK Nico Kruber added a comment -

          or even better, migrate the IllegalArgumentException to IOException

          Show
          NicoK Nico Kruber added a comment - or even better, migrate the IllegalArgumentException to IOException
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user NicoK opened a pull request:

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

          FLINK-5559 let KvStateRequestSerializer#deserializeKeyAndNamespace() throw a proper IOException

          This adds the hint that a deserialisation failure probably results from a `"mismatch in the key/namespace serializers used by the KvState instance and this access"` to all thrown exceptions and throws proper `IOException` instances instead of `IllegalArgumentException`.

          The new unit tests require #3171 to be accepted first on which this PR is also based.

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

          $ git pull https://github.com/NicoK/flink flink-5559

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

          https://github.com/apache/flink/pull/3172.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 #3172



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user NicoK opened a pull request: https://github.com/apache/flink/pull/3172 FLINK-5559 let KvStateRequestSerializer#deserializeKeyAndNamespace() throw a proper IOException This adds the hint that a deserialisation failure probably results from a `"mismatch in the key/namespace serializers used by the KvState instance and this access"` to all thrown exceptions and throws proper `IOException` instances instead of `IllegalArgumentException`. The new unit tests require #3171 to be accepted first on which this PR is also based. You can merge this pull request into a Git repository by running: $ git pull https://github.com/NicoK/flink flink-5559 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3172.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 #3172
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3172#discussion_r97099962

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java —
          @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException
          0,
          serializedKeyAndNamespace.length);

          • K key = keySerializer.deserialize(dis);
          • byte magicNumber = dis.readByte();
          • if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - }
          • N namespace = namespaceSerializer.deserialize(dis);
            + try {
            + K key = keySerializer.deserialize(dis);
            + byte magicNumber = dis.readByte();
            + if (magicNumber != 42) { + throw new IOException("Unexpected magic number " + magicNumber + "."); + }

            + N namespace = namespaceSerializer.deserialize(dis);

          • if (dis.available() > 0) { - throw new IllegalArgumentException("Unconsumed bytes in the serialized key " + - "and namespace. This indicates a mismatch in the key/namespace " + - "serializers used by the KvState instance and this access."); - }

            + if (dis.available() > 0)

            { + throw new IOException("Unconsumed bytes in the serialized key and namespace."); + }
          • return new Tuple2<>(key, namespace);
            + return new Tuple2<>(key, namespace);
            + } catch (IOException e) {
              • End diff –

          Do you think it makes sense to directly throw the final exception in the method body? The reported stack traces are usually already very deep.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on a diff in the pull request: https://github.com/apache/flink/pull/3172#discussion_r97099962 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java — @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException 0, serializedKeyAndNamespace.length); K key = keySerializer.deserialize(dis); byte magicNumber = dis.readByte(); if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - } N namespace = namespaceSerializer.deserialize(dis); + try { + K key = keySerializer.deserialize(dis); + byte magicNumber = dis.readByte(); + if (magicNumber != 42) { + throw new IOException("Unexpected magic number " + magicNumber + "."); + } + N namespace = namespaceSerializer.deserialize(dis); if (dis.available() > 0) { - throw new IllegalArgumentException("Unconsumed bytes in the serialized key " + - "and namespace. This indicates a mismatch in the key/namespace " + - "serializers used by the KvState instance and this access."); - } + if (dis.available() > 0) { + throw new IOException("Unconsumed bytes in the serialized key and namespace."); + } return new Tuple2<>(key, namespace); + return new Tuple2<>(key, namespace); + } catch (IOException e) { End diff – Do you think it makes sense to directly throw the final exception in the method body? The reported stack traces are usually already very deep.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3172#discussion_r97100430

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java —
          @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException
          0,
          serializedKeyAndNamespace.length);

          • K key = keySerializer.deserialize(dis);
          • byte magicNumber = dis.readByte();
          • if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - }
          • N namespace = namespaceSerializer.deserialize(dis);
            + try {
            + K key = keySerializer.deserialize(dis);
            + byte magicNumber = dis.readByte();
            + if (magicNumber != 42) {
              • End diff –

          This was an historic artifact of the Flink 1.1 key and namespace serialization with RocksDB. I think we can remove this completely in a different issue.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on a diff in the pull request: https://github.com/apache/flink/pull/3172#discussion_r97100430 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java — @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException 0, serializedKeyAndNamespace.length); K key = keySerializer.deserialize(dis); byte magicNumber = dis.readByte(); if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - } N namespace = namespaceSerializer.deserialize(dis); + try { + K key = keySerializer.deserialize(dis); + byte magicNumber = dis.readByte(); + if (magicNumber != 42) { End diff – This was an historic artifact of the Flink 1.1 key and namespace serialization with RocksDB. I think we can remove this completely in a different issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uce commented on the issue:

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

          I agree that it is more appropriate to throw the `IOException`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on the issue: https://github.com/apache/flink/pull/3172 I agree that it is more appropriate to throw the `IOException`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3172
          Hide
          uce Ufuk Celebi added a comment -

          Fixed in ef13f48 fd63981 f592d4c (release-1.2), c1c6ef1 3fe2cf5 563c3a4 (master).

          Show
          uce Ufuk Celebi added a comment - Fixed in ef13f48 fd63981 f592d4c (release-1.2), c1c6ef1 3fe2cf5 563c3a4 (master).
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3172#discussion_r97279222

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java —
          @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException
          0,
          serializedKeyAndNamespace.length);

          • K key = keySerializer.deserialize(dis);
          • byte magicNumber = dis.readByte();
          • if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - }
          • N namespace = namespaceSerializer.deserialize(dis);
            + try {
            + K key = keySerializer.deserialize(dis);
            + byte magicNumber = dis.readByte();
            + if (magicNumber != 42) { + throw new IOException("Unexpected magic number " + magicNumber + "."); + }

            + N namespace = namespaceSerializer.deserialize(dis);

          • if (dis.available() > 0) { - throw new IllegalArgumentException("Unconsumed bytes in the serialized key " + - "and namespace. This indicates a mismatch in the key/namespace " + - "serializers used by the KvState instance and this access."); - }

            + if (dis.available() > 0)

            { + throw new IOException("Unconsumed bytes in the serialized key and namespace."); + }
          • return new Tuple2<>(key, namespace);
            + return new Tuple2<>(key, namespace);
            + } catch (IOException e) {
              • End diff –

          it may, but there are other `IOException`s in deeper code, e.g. in `dis.readByte();` and I did not want any special code separating these from our `IOException`s and considered this the least obtrusive variant

          Show
          githubbot ASF GitHub Bot added a comment - Github user NicoK commented on a diff in the pull request: https://github.com/apache/flink/pull/3172#discussion_r97279222 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/query/netty/message/KvStateRequestSerializer.java — @@ -377,22 +376,24 @@ public static Throwable deserializeServerFailure(ByteBuf buf) throws IOException 0, serializedKeyAndNamespace.length); K key = keySerializer.deserialize(dis); byte magicNumber = dis.readByte(); if (magicNumber != 42) { - throw new IllegalArgumentException("Unexpected magic number " + magicNumber + - ". This indicates a mismatch in the key serializers used by the " + - "KvState instance and this access."); - } N namespace = namespaceSerializer.deserialize(dis); + try { + K key = keySerializer.deserialize(dis); + byte magicNumber = dis.readByte(); + if (magicNumber != 42) { + throw new IOException("Unexpected magic number " + magicNumber + "."); + } + N namespace = namespaceSerializer.deserialize(dis); if (dis.available() > 0) { - throw new IllegalArgumentException("Unconsumed bytes in the serialized key " + - "and namespace. This indicates a mismatch in the key/namespace " + - "serializers used by the KvState instance and this access."); - } + if (dis.available() > 0) { + throw new IOException("Unconsumed bytes in the serialized key and namespace."); + } return new Tuple2<>(key, namespace); + return new Tuple2<>(key, namespace); + } catch (IOException e) { End diff – it may, but there are other `IOException`s in deeper code, e.g. in `dis.readByte();` and I did not want any special code separating these from our `IOException`s and considered this the least obtrusive variant

            People

            • Assignee:
              NicoK Nico Kruber
              Reporter:
              NicoK Nico Kruber
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development