[GitHub] flink pull request #6206: [FLINK-9654] [core] Changed the check for anonymou...

[zsolt-donca <gi...@git.apache.org>]

GitHub user zsolt-donca opened a pull request:

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

    [FLINK-9654] [core] Changed the check for anonymous classes to avoid InternalError

    …SI-2034.
    
    ## What is the purpose of the change
    
    This pull request avoids triggering [SI-2034](https://issues.scala-lang.org/browse/SI-2034) for Scala classes that are defined inside of Scala objects. The issue will be fixed only when Scala is will be officially supported by Java 9, as, after all, it's fixed in Java 9: https://bugs.openjdk.java.net/browse/JDK-8057919.
    
    As explained in [FLINK-9654](https://issues.apache.org/jira/browse/FLINK-9654), whenever there is a custom `TypeSerializer` implementation that, when serialized, has in its object graph a reference to a class that triggers [SI-2034](https://issues.scala-lang.org/browse/SI-2034), it makes the task manager instance fail, potentially bringing down the entire Flink cluster.
    
    ## Brief change log
      - made the classname-related checks happen *before* the call to `Class.isAnonymousClass`, after all, there is no reason to call it if we can know it just by looking at the class name;
      - added the check for "$macro$" in the name checks, as, after all, macro-generated classes are always anonymous;
      - wrapped the call to `isAnonymousClass` into a try-catch block, to catch the `InternalError` that the issue might trigger.
    
    ## Verifying this change
    
    This change is a trivial rework / code cleanup without any test coverage.
    
    ## 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 changed class annotated with `@Public(Evolving)`: no
      - The serializers: no
      - The runtime per-record code paths (performance sensitive): no
      - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
      - The S3 file system connector: no
    
    ## Documentation
    
      - Does this pull request introduce a new feature? no
      - If yes, how is the feature documented? not applicable


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

    $ git pull https://github.com/zsolt-donca/flink FLINK-9654-internal-error-fix

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

    https://github.com/apache/flink/pull/6206.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 #6206
    
----
commit fbad06a398fe58f8d312e0ed4dc6bdd31ac65d08
Author: Zsolt Donca <zs...@...>
Date:   2018-06-25T07:43:29Z

    FLINK-9654 Changed the way we check if a class is anonymous to avoid SI-2034.

----


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[tzulitai <gi...@git.apache.org>]

Github user tzulitai commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    Thanks for looking into this @zsolt-donca.
    The changes looks good to me, but as @yanghua mentioned, a test to cover the previous bug would be nice here and allow the reviewer to understand directly that it is a required fix.


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[yanghua <gi...@git.apache.org>]

Github user yanghua commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    hi @zsolt-donca I have seen the Travis build error log, the failed reason is not because of your code. This PR looks good, but if you can add a test for `isAnonymousClass` method, that would be better.
    cc @tillrohrmann 


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[zsolt-donca <gi...@git.apache.org>]

Github user zsolt-donca commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    Thanks for the review! I will update the MR with tests covering the bug.


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[zsolt-donca <gi...@git.apache.org>]

Github user zsolt-donca commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    @tillrohrmann looks good to me. The class `Foo.Bar.Foobar` should trigger the issue. Thanks for writing the test!


---

[GitHub] flink pull request #6206: [FLINK-9654] [core] Changed the check for anonymou...

[asfgit <gi...@git.apache.org>]

Github user asfgit closed the pull request at:

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


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[zsolt-donca <gi...@git.apache.org>]

Github user zsolt-donca commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    It would be great if somebody could help me understand what went wrong with the tests. I tried running the same tests locally (kafka connector-related tests), but they pass for me. Is this some kind of instability?


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[tillrohrmann <gi...@git.apache.org>]

Github user tillrohrmann commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    @zsolt-donca I actually added some test to my branch https://github.com/tillrohrmann/flink/commits/FLINK-9654. If this should cover your fix, then I could merge your PR with this test. What do you think?


---

[GitHub] flink issue #6206: [FLINK-9654] [core] Changed the check for anonymous class...

[tillrohrmann <gi...@git.apache.org>]

Github user tillrohrmann commented on the issue:

    https://github.com/apache/flink/pull/6206
  
    Glad to hear. Then I will merge your PR with this test. Thanks for fixing this problem @zsolt-donca.


---