[jira] [Comment Edited] (HBASE-21456) Make WALFactory only used for creating WALProviders
[ https://issues.apache.org/jira/browse/HBASE-21456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16804375#comment-16804375 ] Ankit Singhal edited comment on HBASE-21456 at 3/28/19 10:34 PM: - bq, I see a lot of changes where we just pass down null, true as the third and fourth arguments to createReader. Would it make sense to overload the createReader method, creating a 2-arg and a 4-arg version? Yep, As these are WalProvider interface method so overloading may not be possible until we do it through WalProviderFactory or specific implementation. But anyways, these methods argument list is gonna change in the next patch for WAL API. bq. Overall, I think this looks good. I think it helps clear up some of the ambiguity we had before. Have you investigated if the test values are related to your patch? I have not seen these failures with other pre-commit jobs(for other patches) so could be related to this, but when I ran them locally, they are passing without any issue. so I'll keep an eye on them specifically when working through my another patch. Thanks [~elserj] for reviewing the patch, If everything looks ok for now, can we commit this? was (Author: an...@apache.org): bq, I see a lot of changes where we just pass down null, true as the third and fourth arguments to createReader. Would it make sense to overload the createReader method, creating a 2-arg and a 4-arg version? Yep, As these are WalProvider interface method so overloading may not be possible until we do it through WalProviderFactory or specific implementation. But anyways, these methods argument list is gonna change in the next patch for WAL API. bq. Overall, I think this looks good. I think it helps clear up some of the ambiguity we had before. Have you investigated if the test values are related to your patch? I have not seen these failures with other pre-commit jobs(for other patches) so could be related to this, but when I ran them locally, they are passing without any issue. so I'll keep an eye on them specifically when working through my another patch. Thanks [~elserj] for review the patch, If everything looks ok for now , can we commit this? > Make WALFactory only used for creating WALProviders > --- > > Key: HBASE-21456 > URL: https://issues.apache.org/jira/browse/HBASE-21456 > Project: HBase > Issue Type: Sub-task > Components: wal >Affects Versions: HBASE-20952 >Reporter: Josh Elser >Assignee: Ankit Singhal >Priority: Major > Fix For: HBASE-20952 > > Attachments: HBASE-21456.HBASE-20952.001.patch, > HBASE-21456.HBASE-20952.002.patch, HBASE-21456.HBASE-20952.003.patch, > HBASE-21456.HBASE-20952.wip.patch > > > As a Factory, WALFactory should only have one job: creating instances of > WALProvider. > However, over the years, it has been a landing place for lots of wal-related > methods (e.g. constructing readers, WALEntryStream, and more). We want all of > this to live in the WALProvider. > We can do this in two steps: have the WALFactory methods invoke the method on > the WALProvider (handled elsewhere), then here we can replace usage of the > WALFactory "wrapper methods" with the WALProvider itself. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Comment Edited] (HBASE-21456) Make WALFactory only used for creating WALProviders
[
https://issues.apache.org/jira/browse/HBASE-21456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16788279#comment-16788279
]
Ankit Singhal edited comment on HBASE-21456 at 3/8/19 8:39 PM:
---
bq. Why have both a public constructor and the static {{getInstance()}} method?
Can we consolidate in one direction?
Yes, agreed, we don't need both, working on refactoring the same along with the
current work (expecting to have public constructor instead of getInstance() and
move same factory object all around the code).
bq.Trying to think about how to help folks like Stack and Reid: what about an
issue to start updating the book? A (short) chapter on how we see WALs working
that briefly illustrate the design would help (and we'd probably write such
things later, anyways). This would be helpful to cover things like
Good point, will be glad to work on this , I'll pick this up immediately after
completing the current required API work for reader/writer.
How about committig this one in branch as will be producing the another patch
on the top of it for another Jira which will take care of above review comment
as well?
was (Author: an...@apache.org):
{quote}Why have both a public constructor and the static {{getInstance()}}
method? Can we consolidate in one direction?\{quote}
Yes, agreed, we don't need both, working on refactoring the same along with the
current work (expecting to have public constructor instead of getInstance() and
move same factory object all around the code).
{quote}Trying to think about how to help folks like Stack and Reid: what about
an issue to start updating the book? A (short) chapter on how we see WALs
working that briefly illustrate the design would help (and we'd probably write
such things later, anyways). This would be helpful to cover things like:\{quote}
Good point, will be glad to work on this , I'll pick this up immediately after
completing the current required API work for reader/writer.
How about committig this one in branch as will be producing the another patch
on the top of it for another Jira which will take care of above review comment
as well?
> Make WALFactory only used for creating WALProviders
> ---
>
> Key: HBASE-21456
> URL: https://issues.apache.org/jira/browse/HBASE-21456
> Project: HBase
> Issue Type: Sub-task
> Components: wal
>Reporter: Josh Elser
>Assignee: Ankit Singhal
>Priority: Major
> Attachments: HBASE-21456.HBASE-20952.001.patch,
> HBASE-21456.HBASE-20952.wip.patch
>
>
> As a Factory, WALFactory should only have one job: creating instances of
> WALProvider.
> However, over the years, it has been a landing place for lots of wal-related
> methods (e.g. constructing readers, WALEntryStream, and more). We want all of
> this to live in the WALProvider.
> We can do this in two steps: have the WALFactory methods invoke the method on
> the WALProvider (handled elsewhere), then here we can replace usage of the
> WALFactory "wrapper methods" with the WALProvider itself.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
