[GitHub] sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-381868889 retest this please 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
[GitHub] sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377391260 > @sijie if you are ok with my comment regarding currentLogId, I can send next iteration change. go for it now. I will think of how to clean it up later. 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
[GitHub] sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-376631793 @ivankelly : appreciate you are working on this change. Here are my thought below: > If there are circular dependencies, then the abstraction is broken from the very start. I've commented on how to fix it, and pushed a sketched patch to master...ivankelly:entrylogmanager which deals with a lot of the issues. if `EntryLogManager` is an inner class (as what Charan) proposed, the abstraction is not *broken*. It is just need refactor later. I am saying "circular dependencies" is for what you said moving `EntryLogManager` out of the EntryLogger class. You need to move a lot of common code around to break the circular depenency, which I think that is too much for this PR or not worth at this moment. Because that is taking this PR into a whole "refactor" story, which I don't think we should take, because 1) we are losing the focus 2) it moves code around, make things a bit hard to review 3) the movement can be tricky, you don't really know if that works for multiple entry log. The approach what I am suggesting is focusing on the interface - what is the right methods that EntryLogManager need for both single entry log and multiple entry logs. Get the interface done, Get the multiple entry log implementation, and do the refactor to move entry log manager out. That is the most practical approach than the reverse. Otherwise we might end up moving code around and eventually found that this movement doesn't work for multiple entry log approach. There is no real sense talking about inner class vs outer class before we get an interface abstraction that can work for both single entry log and multiple entry logs. That is my thought. but if you really feel strong about it, go for it. 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
[GitHub] sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-376311538 @ivankelly : > Move into its own java file. EntryLogger is already huge. > Move into it's own file. I see you left a few comments around moving entry log manager out side of the entry logger class. It is a bit difficult in this patch, because entry log manager is sharing common stuffs (such as buffered log channels) between implementations. I would suggest not doing this large refactor (by moving stuffs out of entry logger). Let this PR focus on the interface abstraction and make EntryLogger responsible for managing EntryLogManager. Once we agree on the interface, we can do the cleanup by moving the interface out and have some common abstract implementation in subsequent changes. Let's focus on abstracting interface at this PR, not to complicate stuffs. 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
