You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StefanRRichter <gi...@git.apache.org> on 2017/07/18 15:17:34 UTC

[GitHub] flink pull request #4360: [FLINK-7220] [checkpoints] Update RocksDB dependen...

GitHub user StefanRRichter opened a pull request:

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

    [FLINK-7220] [checkpoints] Update RocksDB dependency to 5.5.1

    This PR updates the RocksDB dependency to 5.5.1. Further changes:
    
    - the more performant merge operator is used via `setMergeOperatorName("stringappendtest")`.
    - the option `.setDisableDataSync(...)` was removed from RocksDB. Usage is also removed from Flink.

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

    $ git pull https://github.com/StefanRRichter/flink rocksUpdate

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

    https://github.com/apache/flink/pull/4360.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 #4360
    
----
commit 5071d6da08af84dc7a160ceba7063df7828ee6f8
Author: Stefan Richter <s....@data-artisans.com>
Date:   2017-05-29T09:11:41Z

    WIP

----


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Thanks for the review @StephanEwen . I changed my PR according to your comments. Will merge now.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    We should still be able to switch back to the original RocksDB dependency. The licensing issue should also apply to frocksdb.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Just noticed that the license is not updated in the 5.5.1 branch, but only in master. So I guess this means we have to wait for a release that has the updated Apache compatible license?


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Agree. There are two very non-descriptive commits in the master now.
    Let's try to avoid that in the future...


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Merged in 219ae33d36


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Yes, that is correct, I kindly asked them to also update their license in the releases on their issue tracker https://github.com/facebook/rocksdb/issues/2605.
    
    I will update the PR as soon as this version is available from Maven Central.
    



---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    @uce @StephanEwen we had a bit of continued discussion in the JIRA. Can we add a pre-commit hook to the Apache repo to validate the format of the summary line?


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Looks good, +1 with request for minor change:
    
    I would pull out the setting of the merge operator from the option factories. We should not allow users to override (or forget to set it), because we make a hard assumption on the fact that exactly that merge operator is configured.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    CC @aljoscha - suggesting to add this change also to the 1.3.2 release.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Thanks for the pointer Greg. I didn't look at the JIRA issue. I like the idea of automating this but I would go with a pre-push hook instead of a pre-commit hook. It could be annoying during local dev if we force every commit to have the format already. I'm not sure whether INFRA allows this or not though.
    
    @StefanRRichter Sure, can happen. 😊 


---
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 pull request #4360: [FLINK-7220] [checkpoints] Update RocksDB dependen...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter closed the pull request at:

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


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    @uce I know, but this was not intentional. I squashed them but pushed the wrong (unsquashed) branch because of my shell history.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Version 5.5.5 is already published, so from my side this is now good to merge. Can somebody give me another +1?


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    RocksDB 5.5.5 looks to include the desired licensing.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by uce <gi...@git.apache.org>.
Github user uce commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    @StefanRRichter As mentioned by Greg, we should either squash follow up commits like `Stephan's comments` into their parent or tag them similar to the main commit with a more descriptive message.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    The merge looks to have not been squashed.


---
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 #4360: [FLINK-7220] [checkpoints] Update RocksDB dependency to 5...

Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:

    https://github.com/apache/flink/pull/4360
  
    Very good news, thanks for taking care of that!


---
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.
---