thangTang commented on PR #5016: URL: https://github.com/apache/hbase/pull/5016#issuecomment-1425170220
> > > I tried to refactoring a bit but the implementation of ProtobufLogReader is too complicated. I think we'd better abstract two types of WAL.Reader for reading WAL file. One is StreamingReader, which is used in most cases, for example, WAL splitting, WAL printing, etc, where we only need to read the file once and usually for closed WAL files. There is no need to support reset and seek. The other is TailingReader, which is used by Replication, where we need to support reset and seek, and also we need to tell the upper layer whether we need to reset the compress context when calling reseting. The logic will be more complicated as we need to consider the requirements for tailing a WAL file which is currently being written. The refactoring will be a bit big so I do not think we should apply it to branch-2.5 and branch-2.4. So let's apply the simple fix here and file another issue to implement the big refactoring. > > > Thanks. > > > > > > I understand that this is a complicated and dirty job, I am ashamed that I didn't solve it thoroughly before... But by the way, just for this PR, Would you mind taking a look at https://issues.apache.org/jira/browse/HBASE-26850 and #4233? At that time, I thought that it could not fundamentally solve the problem, so I did not continue to push forward, but these two patches seem a bit similar? The difference is that I changed the implementation of addEntry. > > I think this can be done step by step. First, we apply the patch here to fix the problem first, where the performance maybe bad than before. And then, we refactor the Reader, to introduce two types of Reader, so we can focus on how to improve the performance of tailing the WAL file which is being written currently in Repliaction without affecting the WAL splitting logic. Then we could try to introduce fine-grained control on whether we should reconstruct the dictionary, and finally, we could try to improve the LRUDictionary to support checkpoint and rollback, and do a checkpoint at a proper place and use rollback instead of clear and reconstruct, to get all the performance back. > > WDYT? > > Thanks. Make sense. +1 from me. -- 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]
