aliehsaeedii commented on code in PR #21674:
URL: https://github.com/apache/kafka/pull/21674#discussion_r2993320469
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/AbstractSegments.java:
##########
@@ -59,7 +59,9 @@ abstract class AbstractSegments<S extends Segment> implements
Segments<S> {
protected abstract S createSegment(long segmentId, String segmentName);
- protected abstract void openSegmentDB(final S segment, final
StateStoreContext context);
+ protected void openSegment(final S segment, final StateStoreContext
context) {
+ segment.openDB(context.appConfigs(), context.stateDir());
+ }
Review Comment:
It seems like a small benefit with a high cost. Such methods are simple,
obvious, and don't hurt maintainability. So better to keep them in subclasses
and dont do hacking?
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/KeyValueSegments.java:
##########
@@ -40,11 +40,6 @@ protected KeyValueSegment createSegment(final long
segmentId, final String segme
return new KeyValueSegment(segmentName, name, segmentId, position,
metricsRecorder);
}
- @Override
- protected void openSegmentDB(final KeyValueSegment segment, final
StateStoreContext context) {
- segment.openDB(context.appConfigs(), context.stateDir());
- }
Review Comment:
I left my comment above. It's up to you.
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/KeyValueSegment.java:
##########
@@ -49,17 +46,6 @@ public void destroy() throws IOException {
Utils.delete(dbDir);
}
- @Override
- public synchronized void deleteRange(final Bytes keyFrom, final Bytes
keyTo) {
- super.deleteRange(keyFrom, keyTo);
- }
-
- @Override
- public void openDB(final Map<String, Object> configs, final File stateDir)
{
- super.openDB(configs, stateDir);
- // skip the registering step
- }
Review Comment:
On one hand, makes sense to me, on the other hand, was nt Object Oriented
Programming designed to work this way? I mean having private, protected, .. and
then overriding and ... (encapsulation concept)
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java:
##########
@@ -181,8 +181,11 @@ public void init(final StateStoreContext stateStoreContext,
false);
}
+ // This method must be public, to allow us to share code inside
AbstractSegments#openSegmentDB(...)
+ // We declare the same method on interface Segment.openDB(...) to make it
accessible
+ // and interface methods are `public`
@SuppressWarnings("unchecked")
- void openDB(final Map<String, Object> configs, final File stateDir) {
+ public void openDB(final Map<String, Object> configs, final File stateDir)
{
Review Comment:
I'm in the side of keeping encapsulation as I mentioned above.
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/LogicalKeyValueSegments.java:
##########
@@ -72,7 +72,7 @@ protected LogicalKeyValueSegment createSegment(final long
segmentId, final Strin
}
@Override
- protected void openSegmentDB(final LogicalKeyValueSegment segment, final
StateStoreContext context) {
+ protected void openSegment(final LogicalKeyValueSegment segment, final
StateStoreContext context) {
// no-op -- a logical segment is just a view on an underlying physical
store
}
Review Comment:
It means we haven't fully eliminated the need for subclasses to know about
this method (I'm supporting my first cmment;-))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
