reddycharan commented on a change in pull request #1319: Issue 1316: A bookie
with non-writable dirs should be able to start in readonly mode
URL: https://github.com/apache/bookkeeper/pull/1319#discussion_r179691652
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
##########
@@ -398,7 +401,11 @@ synchronized long getLeastUnflushedLogId() {
}
synchronized long getCurrentLogId() {
- return logChannel.getLogId();
+ return currentLogId;
+ }
+
+ BufferedLogChannel getCurrentLogChannel() {
Review comment:
yeah these aren't public methods, but since there are packaged methods there
is a possibility that classes in this package might use it, just like
getcurrentlogid is called from SortedLedgerStorage, which I'm afraid of. For
these classes, the type of the entrylogmanager should be transparent and at
that time entrylogmanager for entrylogperledger should also provide that api.
But still I don't understand why do you need one more variable -
currentLogId and one more method - getcurrentlogchannel, when your test cases
are just checking if it is null or not null. It is extra burden to keep these
vaiables in sync currentlogchannel and currentLogId and maintain one more
method - getcurrentlogchannel (which I don't think should be exposed in the
first place).
If you still feel strong about it, then let's have just currentLogChannel
variable and getCurrentLogChannel method. We can remove currentLogId variable
and it's corresponding getter. (for my entrylogmanager manager I would return
null for this method).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
