[
https://issues.apache.org/jira/browse/HBASE-5937?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13466590#comment-13466590
]
stack commented on HBASE-5937:
------------------------------
bq. ...Making of HLog an interface and redesigning it all at once might be too
messy.
Sounds fair enough.
On the FSHLog cast, lets deal w/ these methods made public for testing later,
after patch goes in.
Methods like canGetCurReplicas are in a bit of a gray area at mo; they are
package private but I see you do the cast because they are not in the
Interface. Again, we can deal w/ these later (canGetCurReplicas is an
interesting one.... you know why we have it in hdfs? Let us know if you don't
know. Might help you figuring whether it belongs in public Interface or not.
hasDeferredEntries is another.
More review...
Why we skip factory here?
{code}
- HLog hlog = new HLog(fs, new Path(rootRegionDir, "wals"),
- new Path(rootRegionDir, "old.wals"), getConf()) {
+ HLog hlog = new FSHLog(fs, rootRegionDir, "wals", getConf()) {
{code}
You think getDir doesn't belong in Interface?
{code}
- Path dir = hlog.getDir();
+ Path dir = ((FSHLog) hlog).getDir();
{code}
You would like to hide notion of dir? Let it be implementation detail? Not
let it out in Interface?
So, getReader will take a FS implementation?
{code}
+ HLog.Reader reader =
HLogFactory.createReader(wal.getFileSystem(getConf()),
+ wal, getConf());
{code}
Is above right? Taking fs and an HLog implementation? Should we be able to
get the fs from the HLog Interface? Almost ditto for conf? Could ask the HLog
implementation for the configuration its using? Or you just want to do that
fix up later after commit? Ditto for createWriter. Just pass HLog Instance.
This is odd method call... not your doing I'd say:
{code}
- HLog.resetLogReaderClass();
+ HLogFactory.resetLogReaderClass();
{code}
I'd think we'd take the class we want to set reader too as an argument?
On the Interface, I did not realize you could define a class on its inside.
I'd say whatever about Reader and Writer -- they are small and it might be good
keeping all together (or not -- whatever you think), I 'd think we could move
this Entry class out of HLog to its own class.
Just remove this stuff rather than comment out:
{code}
+ //@org.junit.Rule
+ //public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu =
+ // new org.apache.hadoop.hbase.ResourceCheckerJUnitRule();
{code}
HLogUtil.COMPLETE_CACHE_FLUSH should be in HLog not in HLogUtil I'd say.
This stuff you will probably want to hide someday:
getServerNameFromHLogDirectoryName ... its implementation detail... but later.
Should this be in HLogUtil -- getHLogDirectoryName? Seems HLog attribute?
Import in TestRowProcessorEndpoint but unused
Yeah, this is dirty... getServerNameFromHLogDirectoryName... that'd be in an
HLog that you cast to a FSHLog rather than in HLogUtil (I'd think the latter
general util, not particular to an implementation?)
You'll fix WALCoprocessorHost later? So it can load whatever the WAL
implementation, not just FSHLog?
Is this wrong?
{code}
- HLog.Entry entry;
+ FSHLog.Entry entry;
{code}
Is Entry still in the HLog Interface?
This seems like a facility that belongs in HLog rather than out in HLogUtil:
{code}
- keyClass = HLog.getKeyClass(conf);
+ keyClass = HLogUtil.getKeyClass(conf);
{code}
You make a change in WALActionsListener javadoc but no other? Maybe you were
able to get away w/ using HLog only? And not need to specify FSHLog?
... let me submit this in case JIRA loses it on me.
> Refactor HLog into an interface.
> --------------------------------
>
> Key: HBASE-5937
> URL: https://issues.apache.org/jira/browse/HBASE-5937
> Project: HBase
> Issue Type: Sub-task
> Reporter: Li Pi
> Assignee: Flavio Junqueira
> Priority: Minor
> Attachments: 5937-hlog-with-javadoc.txt, HBASE-5937.patch,
> HBASE-5937.patch, HBASE-5937.patch, HBASE-5937.patch, HBASE-5937.patch,
> org.apache.hadoop.hbase.client.TestMultiParallel-output.txt
>
>
> What the summary says. Create HLog interface. Make current implementation use
> it.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
