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

 File path: 
 @@ -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:

With regards,
Apache Git Services

Reply via email to