hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "stack (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5937) Refactor HLog into an interface.
Date Sun, 30 Sep 2012 23:02:08 GMT

    [ 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

Mime
View raw message