You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by gyfora <gi...@git.apache.org> on 2016/04/20 21:36:08 UTC
[GitHub] flink pull request: [FLINK-3798] [streaming] Clean up RocksDB back...
GitHub user gyfora opened a pull request:
https://github.com/apache/flink/pull/1918
[FLINK-3798] [streaming] Clean up RocksDB backend field/method access
The RocksDB state backend uses a lot package private methods and fields which makes it very hard to subclass the different parts for added functionality. I think these should be protected instead.
Also the AbstractRocksDBState declares some methods final when there are use-cases when a subclass migh want to call them.
Just to give you an example I am creating a version of the value state which would keep a small cache on heap. For this it would be enough to subclass the RockDBStateBackend and RocksDBVAlue state classes if the above mentioned changes were made. Now I have to use reflection to access package private fields and actually copy classes due to final methods.
Activity
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gyfora/flink rocks-access
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1918.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 #1918
----
commit a1e323650bd5f603fbf3809fe1fe901cbab1a56b
Author: Gyula Fora <gy...@apache.org>
Date: 2016-04-20T19:33:30Z
[FLINK-3798] [streaming] Clean up RocksDB backend field/method access
----
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1918
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1918#discussion_r60543157
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBValueState.java ---
@@ -64,7 +64,7 @@
* @param stateDesc The state identifier for the state. This contains name
* and can create a default state value.
*/
- RocksDBValueState(ColumnFamilyHandle columnFamily,
+ public RocksDBValueState(ColumnFamilyHandle columnFamily,
--- End diff --
Would it be enough to make this `protected`?
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1918#discussion_r60550239
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBValueState.java ---
@@ -64,7 +64,7 @@
* @param stateDesc The state identifier for the state. This contains name
* and can create a default state value.
*/
- RocksDBValueState(ColumnFamilyHandle columnFamily,
+ public RocksDBValueState(ColumnFamilyHandle columnFamily,
--- End diff --
Oh, for some reason I didn't see that... 😅 Could you go ahead and merge it then?
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1918#issuecomment-212806739
This look good! I had one remark about making the `RocksDBValueState` constructor `protected`.
Could you please merge after addressing 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 pull request: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/1918#issuecomment-212845767
Hotfix time?
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on the pull request:
https://github.com/apache/flink/pull/1918#issuecomment-212834630
ahh the actual classes were not public either, I'm so stupid to have missed that lol... :d
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on a diff in the pull request:
https://github.com/apache/flink/pull/1918#discussion_r60549764
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBValueState.java ---
@@ -64,7 +64,7 @@
* @param stateDesc The state identifier for the state. This contains name
* and can create a default state value.
*/
- RocksDBValueState(ColumnFamilyHandle columnFamily,
+ public RocksDBValueState(ColumnFamilyHandle columnFamily,
--- End diff --
I think thats how it is except for the AbstractRocksDBState :)
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by gyfora <gi...@git.apache.org>.
Github user gyfora commented on a diff in the pull request:
https://github.com/apache/flink/pull/1918#discussion_r60544472
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBValueState.java ---
@@ -64,7 +64,7 @@
* @param stateDesc The state identifier for the state. This contains name
* and can create a default state value.
*/
- RocksDBValueState(ColumnFamilyHandle columnFamily,
+ public RocksDBValueState(ColumnFamilyHandle columnFamily,
--- End diff --
The reason why I made the state constructors public so if you sublcass the RocksDBStatebackend you can still manually instantiate them. You wouldnt be able to call the protected constructor from your package though.
It's no biggie, I can change it if you still think its better as protected.
---
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: [FLINK-3798] [streaming] Clean up RocksDB back...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/1918#discussion_r60547896
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBValueState.java ---
@@ -64,7 +64,7 @@
* @param stateDesc The state identifier for the state. This contains name
* and can create a default state value.
*/
- RocksDBValueState(ColumnFamilyHandle columnFamily,
+ public RocksDBValueState(ColumnFamilyHandle columnFamily,
--- End diff --
Ah I see, you're right. Could you make all of the State constructors public then? Just for 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.
---