Add smoke tests for LedgerHandleAdv and TailingReads#1389
Add smoke tests for LedgerHandleAdv and TailingReads#1389sijie wants to merge 2 commits intoapache:masterfrom
Conversation
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @Slf4j |
There was a problem hiding this comment.
👍 we may need another chance to do a cleanup with Lombok.
ivankelly
left a comment
ivankelly
left a comment
There was a problem hiding this comment.
Couple of nits, but lgtm
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> | ||
| <javac.target>1.8</javac.target> | ||
| <redirectTestOutputToFile>true</redirectTestOutputToFile> |
There was a problem hiding this comment.
Separate change, should be separate PR
There was a problem hiding this comment.
I am changing the variable ONLY in the smoke test module, which is the module I am testing. I am not touching any other places that have this flag. so I am not sure if it really needs to be a separated PR, but if you feel strong, I can do it in a separate PR.
There was a problem hiding this comment.
it's fine for this PR, but in future, changes like this should be separate.
| String zookeeper = BookKeeperClusterUtils.zookeeperConnectString(docker); | ||
| @Cleanup BookKeeper bk = new BookKeeper(zookeeper); | ||
| @Cleanup LedgerHandle writeLh = bk.createLedger(DigestType.CRC32C, PASSWD); | ||
| @Cleanup("shutdown") ExecutorService writeExecutor = Executors.newSingleThreadExecutor( |
There was a problem hiding this comment.
nit: A fixed thread pool with numThreads = 2, would reduce the amout of cleanup.
There was a problem hiding this comment.
it is much clear to have two seperated thread pools with different names for debugging purpose.
|
|
||
| // start the read thread | ||
| readExecutor.submit(() -> { | ||
| long lastEntryId = numEntries - 2; |
There was a problem hiding this comment.
A comment for where the -2 comes from would be useful. Maybe rename to lastExpectedConfirmedEntryId to make it clear its not the last entry written, but the entry before the last entry written, and hence the last confirmed.
| CompletableFuture<Void> writeFuture = new CompletableFuture<>(); | ||
|
|
||
| // start the read thread | ||
| readExecutor.submit(() -> { |
There was a problem hiding this comment.
nit: If you coerce the lambda to a callable, your can assign readFuture directly from submit, and skip the try-catch
Future<?> readFuture = readExecutor.submit((Callable<Void>)() -> ...);
There was a problem hiding this comment.
I wrote it in a try-catch block so that I can write log statements.
| @@ -48,42 +55,143 @@ public class TestSmoke { | |||
| private String currentVersion = System.getProperty("currentVersion"); | |||
|
|
|||
| @Before | |||
There was a problem hiding this comment.
These could be beforeClass and afterClass, since there's no need to have a clean cluster for the different api tests.
There was a problem hiding this comment.
I am not sure beforeClass and afterClass work in arquillian tests. (I could be wrong, but that was my impression from Ali)
There was a problem hiding this comment.
now that you mention it, that could be the case. I think I tried to move the init metadata stuff to beforeClass before, and it refused to work
|
lgtm #shipit |
|
retest this please |
|
the change is about IT tests. All the IT tests passed. so going to ignore CI in order to merge it. |
|
IGNORE CI |
Descriptions of the changes in this PR: Add two tests: - basic writes/reads with LedgerHandleAdv - tailing reads with LedgerHandle Author: Sijie Guo <sijie@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes #1389 from sijie/more_integration_tests
Descriptions of the changes in this PR: Add two tests: - basic writes/reads with LedgerHandleAdv - tailing reads with LedgerHandle Author: Sijie Guo <sijie@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes apache#1389 from sijie/more_integration_tests Signed-off-by: JV Jujjuri <vjujjuri@salesforce.com>
4 participants
Descriptions of the changes in this PR:
Add two tests: