Currently all the code for interleaved ledger storage is in the bookie package. Likewise, the tests are in the bookie package. This makes it unclear what is a test of the ledger storage and what is a test of the bookie in general.
This JIRA is to move both of these into a subpackage of bookie, o.a.b.bookie.ills. BOOKKEEPER-432 will then go into o.a.b.bookie.sls, and each of the tests in the ills package will have a corresponding test in sls.
{color:green}+1 PATCH_APPLIES{color}
{color:green}+1 CLEAN{color}
{color:green}+1 RAW_PATCH_ANALYSIS{color}
. {color:green}+1{color} the patch does not introduce any @author tags
. {color:green}+1{color} the patch does not introduce any tabs
. {color:green}+1{color} the patch does not introduce any trailing spaces
. {color:green}+1{color} the patch does not introduce any line longer than 120
. {color:green}+1{color} the patch does adds/modifies 13 testcase(s)
{color:green}+1 RAT{color}
. {color:green}+1{color} the patch does not seem to introduce new RAT warnings
{color:green}+1 JAVADOC{color}
. {color:green}+1{color} the patch does not seem to introduce new Javadoc warnings
{color:green}+1 COMPILE{color}
. {color:green}+1{color} HEAD compiles
. {color:green}+1{color} patch compiles
. {color:green}+1{color} the patch does not seem to introduce new javac warnings
{color:green}+1 FINDBUGS{color}
. {color:green}+1{color} the patch does not seem to introduce new Findbugs warnings
{color:green}+1 TESTS{color}
. Tests run: 842
{color:green}+1 DISTRO{color}
. {color:green}+1{color} distro tarball builds with the patch
{color:green}+1 Overall result, good!, no -1s{color}
The full output of the test-patch run is available at
{color:red}-1{color} Patch failed to apply to head of branch
Sijie Guo 2013-06-18T00:25:08.557+0000
one question, if you are adding a total different ledger storage, it makes sense to put the interleaved ledger storage into a sub package. but since the skip list ledger storage is based on interleaved ledger storage, I don't see the value of doing this refactor now. I would hold on this refactor until we need it. since we had bunch of patches is based on current package structure, I would suggest getting them merged first before we doing this refactor, otherwise it is quite painful for us to contribute.
Ivan Kelly 2013-06-18T09:23:10.491+0000
{quote}one question, if you are adding a total different ledger storage, it makes sense to put the interleaved ledger storage into a sub package. but since the skip list ledger storage is based on interleaved ledger storage, I don't see the value of doing this refactor now.{quote}
Currently, the journal, the storage, the sync thread and a load of utility classes are in the one package. The storage consists of many classes. The value of this patch is moving them into their own package, so that it's clear which classes belong to storage. The whether we have o.a.b.bookie.storage & o.a.b.bookie.storage.sls or o.a.b.bookie.ills & o.a.b.bookie.sls makes no difference to me. Really the aim is to separate them logically, so that it is clearer what belongs to storage, and consequently, what needs to be tested for storage.
{quote}since we had bunch of patches is based on current package structure{quote}
I assume you mean in the twitter github? These will need to be manually merged anyhow due to other changes (such as the checkpointing).
tbh, I don't mind which we do first, but not having this change in will mean differing adding testing for BOOKKEEPER-432. I'm ok with this as long as a testing jira for BOOKKEEPER-432 becomes a blocker for the 4.3.0 release.
Sijie Guo 2014-01-24T08:30:14.009+0000
[~ikelly] could you rebase you change to latest trunk? so we could start getting this change in.
Ivan Kelly 2014-01-24T14:14:05.534+0000
Actually, i think it'd be better to get the twitter storage stuff in first and refactor later, when the number of changes around storage have reduced a lot.
Sijie Guo 2014-07-26T07:24:13.850+0000
move it to 4.4.0. it is good to move files around at the early stage of a release, rather than at the end of it.
Sijie Guo 2017-06-22T00:29:21.394+0000
postponed to 4.6.0. we should consider a package refactor after 4.5.0.
ivankelly
commented
6 years ago
JIRA: https://issues.apache.org/jira/browse/BOOKKEEPER-613
Reporter: Ivan Kelly @ivankelly
Currently all the code for interleaved ledger storage is in the bookie package. Likewise, the tests are in the bookie package. This makes it unclear what is a test of the ledger storage and what is a test of the bookie in general.
This JIRA is to move both of these into a subpackage of bookie, o.a.b.bookie.ills. BOOKKEEPER-432 will then go into o.a.b.bookie.sls, and each of the tests in the ills package will have a corresponding test in sls.
Comments from JIRA
Hadoop QA 2013-05-27T17:21:16.959+0000
Testing JIRA BOOKKEEPER-613
Patch [0001-BOOKKEEPER-613-Prep-codebase-for-new-ledger-storage.patch|https://issues.apache.org/jira/secure/attachment/12584951/0001-BOOKKEEPER-613-Prep-codebase-for-new-ledger-storage.patch] downloaded at Mon May 27 16:51:01 UTC 2013
{color:green}+1 PATCH_APPLIES{color} {color:green}+1 CLEAN{color} {color:green}+1 RAW_PATCH_ANALYSIS{color} . {color:green}+1{color} the patch does not introduce any @author tags . {color:green}+1{color} the patch does not introduce any tabs . {color:green}+1{color} the patch does not introduce any trailing spaces . {color:green}+1{color} the patch does not introduce any line longer than 120 . {color:green}+1{color} the patch does adds/modifies 13 testcase(s) {color:green}+1 RAT{color} . {color:green}+1{color} the patch does not seem to introduce new RAT warnings {color:green}+1 JAVADOC{color} . {color:green}+1{color} the patch does not seem to introduce new Javadoc warnings {color:green}+1 COMPILE{color} . {color:green}+1{color} HEAD compiles . {color:green}+1{color} patch compiles . {color:green}+1{color} the patch does not seem to introduce new javac warnings {color:green}+1 FINDBUGS{color} . {color:green}+1{color} the patch does not seem to introduce new Findbugs warnings {color:green}+1 TESTS{color} . Tests run: 842 {color:green}+1 DISTRO{color} . {color:green}+1{color} distro tarball builds with the patch
{color:green}+1 Overall result, good!, no -1s{color}
The full output of the test-patch run is available at
. https://builds.apache.org/job/bookkeeper-trunk-precommit-build/347/
Hadoop QA 2013-06-15T00:04:28.902+0000
Testing JIRA BOOKKEEPER-613
Patch [0001-BOOKKEEPER-613-Prep-codebase-for-new-ledger-storage.patch|https://issues.apache.org/jira/secure/attachment/12584951/0001-BOOKKEEPER-613-Prep-codebase-for-new-ledger-storage.patch] downloaded at Sat Jun 15 00:02:34 UTC 2013
{color:red}-1{color} Patch failed to apply to head of branch
Sijie Guo 2013-06-18T00:25:08.557+0000
one question, if you are adding a total different ledger storage, it makes sense to put the interleaved ledger storage into a sub package. but since the skip list ledger storage is based on interleaved ledger storage, I don't see the value of doing this refactor now. I would hold on this refactor until we need it. since we had bunch of patches is based on current package structure, I would suggest getting them merged first before we doing this refactor, otherwise it is quite painful for us to contribute.
Ivan Kelly 2013-06-18T09:23:10.491+0000
{quote}one question, if you are adding a total different ledger storage, it makes sense to put the interleaved ledger storage into a sub package. but since the skip list ledger storage is based on interleaved ledger storage, I don't see the value of doing this refactor now.{quote} Currently, the journal, the storage, the sync thread and a load of utility classes are in the one package. The storage consists of many classes. The value of this patch is moving them into their own package, so that it's clear which classes belong to storage. The whether we have o.a.b.bookie.storage & o.a.b.bookie.storage.sls or o.a.b.bookie.ills & o.a.b.bookie.sls makes no difference to me. Really the aim is to separate them logically, so that it is clearer what belongs to storage, and consequently, what needs to be tested for storage.
{quote}since we had bunch of patches is based on current package structure{quote} I assume you mean in the twitter github? These will need to be manually merged anyhow due to other changes (such as the checkpointing).
tbh, I don't mind which we do first, but not having this change in will mean differing adding testing for BOOKKEEPER-432. I'm ok with this as long as a testing jira for BOOKKEEPER-432 becomes a blocker for the 4.3.0 release.
Sijie Guo 2014-01-24T08:30:14.009+0000
[~ikelly] could you rebase you change to latest trunk? so we could start getting this change in.
Ivan Kelly 2014-01-24T14:14:05.534+0000
Actually, i think it'd be better to get the twitter storage stuff in first and refactor later, when the number of changes around storage have reduced a lot.
Sijie Guo 2014-07-26T07:24:13.850+0000
move it to 4.4.0. it is good to move files around at the early stage of a release, rather than at the end of it.
Sijie Guo 2017-06-22T00:29:21.394+0000
postponed to 4.6.0. we should consider a package refactor after 4.5.0.