You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@flink.apache.org by NicoK <gi...@git.apache.org> on 2017/08/01 08:48:46 UTC

[GitHub] flink pull request #4447: [FLINK-7312][checkstyle] activate checkstyle for f...

GitHub user NicoK opened a pull request:

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

    [FLINK-7312][checkstyle] activate checkstyle for flink/core/memory/*

    ## What is the purpose of the change
    
    Activate checkstyle for `flink/core/memory/*`.
    
    This PR is based on #4446.
    
    ## Brief change log
    
      - adapt the code to the checkstyle rules
      - remove the checkstyle suppression for `"(.*)core[/\\]memory[/\\](.*)"`
    
    ## 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)
    
    ## 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/NicoK/flink flink-7312

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

    https://github.com/apache/flink/pull/4447.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 #4447
    
----
commit c62e793712effbfb53ea6442b5d714a68081f7ec
Author: Nico Kruber <ni...@data-artisans.com>
Date:   2017-07-31T10:06:14Z

    [hotfix] fix some typos

commit d3c4e231a96b6ae133576a74646294749ab3809a
Author: Nico Kruber <ni...@data-artisans.com>
Date:   2017-07-31T12:18:42Z

    [FLINK-7310][core] always use the HybridMemorySegment
    
    Since we'd like to use our own off-heap buffers for network communication, we
    cannot use HeapMemorySegment anymore and need to rely on HybridMemorySegment.
    We thus drop any code that loads the HeapMemorySegment (it is still available
    if needed) in favour of the HybridMemorySegment which is able to work on both
    heap and off-heap memory.
    
    For the performance penalty of this change compared to using HeapMemorySegment
    alone, see this interesting blob article (from 2015):
    https://flink.apache.org/news/2015/09/16/off-heap-memory.html

commit e1de341a82148939314a90fde6f5c289f3dc9a10
Author: Nico Kruber <ni...@data-artisans.com>
Date:   2017-07-31T12:36:56Z

    [FLINK-7311][tests] refrain from using fail(Exception#getMessage())

commit 3839b83d708a48ac789261d86f6f08beb3d65b34
Author: Nico Kruber <ni...@data-artisans.com>
Date:   2017-07-31T13:35:37Z

    [FLINK-7312][checkstyle] activate checkstyle for flink/core/memory/*

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by greghogan <gi...@git.apache.org>.

Github user greghogan commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    @NicoK, just wanted to suggest that (other than trivial or single file modifications) to have separate commits for trailing whitespace and import order, then one or more checkstyle commits as necessary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by greghogan <gi...@git.apache.org>.

Github user greghogan commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    @StephanEwen we can search the diffs for commits with `checkstyle` in the summary for removal of `final`, which only yields ~100 results with only a handful requiring reversion.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by zentol <gi...@git.apache.org>.

Github user zentol commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    Great, if reviewer time is the only concern we can go ahead with #4446, I don't mind spending some free time on it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by NicoK <gi...@git.apache.org>.

Github user NicoK commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    FYI: the `final` keyword on methods in `final` classes is something that is actually forbidden by the checkstyle, hence the change
    
    Regarding the try-catch in tests (actually part of #4446): you're right, I was too quick in arguing that stack traces are not printed - whenever there is a `e.printStackTrace()`, it should work but at least when developing with IntelliJ, this printout is not clickable (during debugging a failure) compared to failing with a proper exception.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by StephanEwen <gi...@git.apache.org>.

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    Final key-words on non-static methods should always be an exception, no matter whether checkstyle marks them as a violation.
    
    Talking to @NicoK offline, he mentioned that there have been commits with such changes merged. What do we do about these now? Are we sure we did not void a statement that the authors put in place to the code to be future proof`?
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by zentol <gi...@git.apache.org>.

Github user zentol commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    FYI, there's no way to configure checkstyle to ignore the final keyword on methods of final classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by StephanEwen <gi...@git.apache.org>.

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    There is also a complete reorg of a test file, removing try/catch blocks. This affects to my knowledge only the way where exactly the stack trace is printed.
    
    I am skeptical about these types of refactorings:
      - Either the reviewer does not really look at them, in which case the danger of accidentally weakening the test coverage by far outweigh the (in my opinion quite subjective and cosmetic) benefits of such a change.
      - Or the reviewer really checks the diff, in which case I would suggest to dedicate that time to other pull requests that bring in actual improvements.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by NicoK <gi...@git.apache.org>.

Github user NicoK commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    Just curious whether we can merge this now - it has been laying around for too long.


---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by StephanEwen <gi...@git.apache.org>.

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    At the first quick glance: This is removing a lot of `final` keywords from various methods.
    
    While one could argue that this keyword is not strictly necessary (the class as a whole is final), I put them there on purpose when writing this originally, to "document" the intention that these methods should not be ever overridden, regardless of whether the class as a whole would be subclass-able.
    
    I would vote to leave this in place, actually. While actually improving / cleaning up bad code is a nice thing to do as part of reworking parts, I think this is good practice to not try and just change things that could be changed, just for the sake of changing them, or because of a personal view of "what would look nicer".
    There is frequently a reason why things are as they are, especially in modules like the memory segments that were written very carefully and consciously.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by NicoK <gi...@git.apache.org>.

Github user NicoK commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    :) in here, however, I had to add an exception for these `final` keywords which may be removed when #4458 is merged.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by StephanEwen <gi...@git.apache.org>.

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    Thinking more about this, I think we should modify the checkstyle to not force us to remove such `final` keywords. While being redundant in the current "snapshot" of the code, they may not be redundant in the future. I think there is a big upside of writing code in a "future proof" way.
    
    The modifiers in this class are a good example of that - the JIT friendlyness of the methods is so important that they should not be affected by future changes the class hierarchy.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by NicoK <gi...@git.apache.org>.

Github user NicoK commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    oh, yes - maybe I should rebase this one and exclude #4445 commits since there, we may actually investigate further


---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by zentol <gi...@git.apache.org>.

Github user zentol commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    For now I'd rather stick to adding suppressions for the files where we deem it important, which is easy to do btw. If the resulting set of suppressions is small let's stick with it, and revisit the discussion otherwise.
    
    We've already covered large parts of the code-based with the existing rules, I'd rather stick to it for the remainder, with exceptions where necessary, to increase consistency.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by NicoK <gi...@git.apache.org>.

Github user NicoK commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    FYI: I just rebased this PR onto current `master` to make this mergable and support further extensions


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...

Posted by StephanEwen <gi...@git.apache.org>.

Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4447
  
    This seems to be subsumed by #4445 - is that correct?


---