Add event listeners to RocksJava #7425
Conversation
alucarded
force-pushed
the
listeners
branch
4 times, most recently
from
dfb1eaa
to
995a1c5
Compare
|
As regards fix to RTTI issue (avoid dynamic_cast) added in the commit: Another solution (which would seem better at first glance) would be to hold native object EventListenerJniCallback for AbstractEventListener Java class and static_cast it to EventListener type pointer when setting listeners. Then, when getting listeners, we would reinterpret_cast EventListener pointers to EventListenerJniCallback pointers to get the jobject on whhich we would call callback method. Unfortunately... ...we cannot static_cast from derived to base class pointer Even if we redesign EventListenerJniCallback to have single inheritance (JniCallback would be a member class) we will not get standard-layout class (because of virtual methods in EventListener) and thus EventListener and EventListenerJniCallback would not be pointer-interconvertible. |
There was a problem hiding this comment.
We have been through several rounds of internal review with @alucarded already here at Evolved Binary. I am happy this PR is now in a good shape.
Unless anyone has some suggestions about the RTTI issue for avoiding dynamic_cast, then I think it is good to merge.
@pdillinger @siying any comments?
If you're considering reinterpret_cast, I assume you know it's an EventListenerJniCallback object. If the jlong came from an EventListener*, wouldn't you want to reinterpret_cast from jlong back to EventListener* and static_cast from EventListener* to EventListenerJniCallback*? |
|
@pdillinger It all makes sense now... For some reason I assumed I cannot downcast in this case, which is actually false. I am making the change. Thank you. |
alucarded
force-pushed
the
listeners
branch
from
995a1c5
to
ab042f8
Compare
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| * @param flushJobInfo the flush job info, contains data copied from | ||
| * respective native structure. | ||
| */ | ||
| void onFlushCompleted(final RocksDB db, final FlushJobInfo flushJobInfo); |
There was a problem hiding this comment.
Great Work Do you mind move onFlushBegin before onFlushCompleted ?
| @@ -5,6 +5,8 @@ | |||
|
|
|||
| package org.rocksdb; | |||
|
|
|||
| import java.util.Objects; | |||
There was a problem hiding this comment.
why import this package?
There was a problem hiding this comment.
or just say "unused import"
OnTableFileCreationStarted, OnColumnFamilyHandleDeletionStarted, OnExternalFileIngested
Utility class for tests, inherits from AbstractEventListener and adds a native method, which invokes all native callback methods.
Shift 64 bit value instead of shifting 32 bit value and then casting it implicitly to 64 bit value.
|
@alucarded has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jay-zhuang merged this pull request in 6528ecc. |
Summary: Allows adding event listeners in RocksJava. * Adds listeners getter and setter in `Options` and `DBOptions` classes. * Adds `EventListener` Java interface and base class for implementing custom event listener callbacks - `AbstractEventListener`, which has an underlying native callback class implementing C++ `EventListener` class. * `AbstractEventListener` class has mechanism for selectively enabling its callback methods in order to prevent invoking Java method if it is not implemented. This decreases performance cost in case only subset of event listener callback methods is needed - the JNI code for remaining "no-op" callbacks is not executed. * The code is covered by unit tests in `EventListenerTest.java`, there are also tests added for setting/getting listeners field in `OptionsTest.java` and `DBOptionsTest.java`. Pull Request resolved: facebook#7425 Reviewed By: pdillinger Differential Revision: D24063390 Pulled By: jay-zhuang fbshipit-source-id: 508c359538983d6b765e70d9989c351794a944ee
Allows adding event listeners in RocksJava.
OptionsandDBOptionsclasses.EventListenerJava interface and base class for implementing custom event listener callbacks -AbstractEventListener, which has an underlying native callback class implementing C++EventListenerclass.AbstractEventListenerclass has mechanism for selectively enabling its callback methods in order to prevent invoking Java method if it is not implemented. This decreases performance cost in case only subset of event listener callback methods is needed - the JNI code for remaining "no-op" callbacks is not executed.EventListenerTest.java, there are also tests added for setting/getting listeners field inOptionsTest.javaandDBOptionsTest.java.