-
Notifications
You must be signed in to change notification settings - Fork 902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 1626: Add tests for LedgerUnderredplicationManager#markLedgerUnderreplicatedAsync #1629
base: master
Are you sure you want to change the base?
Changes from 2 commits
54ffc3c
4c5d965
d575ac8
0ad959d
66086db
0d2dadf
3768be0
51e6281
0063516
fc96ab5
8b68c83
416e8da
e42d91c
0582692
9cfa787
6973534
b0ba8c9
ffe8db3
2df24d7
08685cc
5966248
a72c53e
0d88710
17b0f8e
3a6e6de
9f6e2b8
5419bea
9dd159e
5af14e2
e3b58ea
3237329
42b3b34
938d0cf
39ee7fd
5138d1b
feae09e
5381e35
92f283f
5dca8ab
b18fb0c
f76fcef
d2492bd
22bacaa
5ba1f7d
7187e8f
6ba16f5
4dd15cf
23bfe50
9a69810
3943c86
4bc798b
dce5f74
8b77121
4473187
06146a7
55673a4
4de5b95
04c190c
070c3b6
43ce06c
42b493a
e1929ac
e3599f3
f7c0db1
f259a1a
c1b5b35
b3b545a
1e5e8ad
156fcc0
ebfd0dc
2ac7aa9
a811b2a
8690f22
0dbad3b
83052d8
2e28b9b
11ebb77
a7519b5
a78a946
6e2728c
d23ef50
bb3fc35
7889438
a564dc4
d9ef7c5
fc03d41
b3c7a15
ea904b3
9864b23
0297acc
b1fa1f0
238f34f
93afa63
0485115
07a01e0
7671823
081cb07
18d9000
62f60a6
0f0d730
0936939
dcb5785
3544a0e
89c8764
524d92d
2559bbe
0e265c2
d3cea9f
3d4fbd7
18ea24c
c9e249c
2290fca
d9f1985
bb6b05f
5ce34f4
3cf8942
87731a1
0f297b3
736eed6
32fdbb9
28ebdd7
bbc37d0
d5015fc
8492680
548b821
3c1d8aa
7f88df2
ff117d0
08148cf
24834d9
22b6a87
27d9b12
911d98b
f0e3e1e
40ff981
bc7f317
b4eab2f
5a191ad
c39dd1c
115f7c2
f10550c
cc32363
3d1e888
0212417
5dc1ff7
9c8e67e
f327200
96fc7ff
c53c81a
c31e8f0
b78da6f
b37b8a6
355d046
fcfdeac
6f9b05b
3751934
78b6957
e8f8a84
024ad62
cde3c19
8a689f3
c6f77a9
8236257
b8eb4a7
f20c440
95aa7e4
50f9963
0b79936
c504732
c83dd12
b5bdcf4
b189b26
063833d
95e5b24
ae5a0fd
affbee7
729a984
514d026
8d4a9fb
a24259b
83d57ab
5084b69
34344e7
807d94d
46233fa
6012972
d115386
92dfb95
c5359b5
2b3247d
3229970
a28837b
a9524d1
022527c
fda3862
949acce
eff109a
9a00eae
d7254ab
307ff7d
e9000b2
6f928d4
5f09dd6
1c7caff
dd55179
1c534b0
d1a7acb
10dfe37
77e73a2
1523146
bd9ea1a
582f271
52320ac
346042c
77cf3f9
4bb9acb
03a9f20
8214b2d
a1926ca
ca99d73
a680f85
593105e
64bea62
d01a817
2fa75d8
b6c7949
7a8297b
67c870f
b45522c
f91ab8d
d7b795c
7aa87b0
603b8b4
74500db
3bc94ed
ff1c230
e25e3f3
4421a8e
264025a
f12b0be
11fcaea
544b34c
d9f8119
b70bb27
d04531c
9fc262d
6af21b1
4e1a30c
c30ab3c
ad48ed4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,244 @@ | ||
/* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.bookkeeper.meta; | ||
|
||
import static java.nio.charset.StandardCharsets.UTF_8; | ||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assert.fail; | ||
|
||
import com.google.common.collect.Lists; | ||
import com.google.common.collect.Sets; | ||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.concurrent.CompletableFuture; | ||
import lombok.Cleanup; | ||
import org.apache.bookkeeper.common.concurrent.FutureUtils; | ||
import org.apache.bookkeeper.conf.ServerConfiguration; | ||
import org.apache.bookkeeper.proto.DataFormats.UnderreplicatedLedgerFormat; | ||
import org.apache.bookkeeper.replication.ReplicationException; | ||
import org.apache.bookkeeper.suites.BookKeeperClusterTestSuite; | ||
import org.apache.bookkeeper.util.ZkUtils; | ||
import org.apache.bookkeeper.zookeeper.BoundExponentialBackoffRetryPolicy; | ||
import org.apache.bookkeeper.zookeeper.ZooKeeperClient; | ||
import org.apache.commons.lang3.StringUtils; | ||
import org.apache.zookeeper.CreateMode; | ||
import org.apache.zookeeper.KeeperException; | ||
import org.apache.zookeeper.KeeperException.NoNodeException; | ||
import org.apache.zookeeper.ZooKeeper; | ||
import org.junit.After; | ||
import org.junit.AfterClass; | ||
import org.junit.Before; | ||
import org.junit.BeforeClass; | ||
import org.junit.Test; | ||
|
||
/** | ||
* Unit test {@link ZkLedgerUnderreplicationManager}. | ||
*/ | ||
public class ZkLedgerUnderreplicationManagerTest extends BookKeeperClusterTestSuite { | ||
|
||
@BeforeClass | ||
public static void setUpCluster() throws Exception { | ||
BookKeeperClusterTestSuite.setUpCluster(0); | ||
} | ||
|
||
@AfterClass | ||
public static void tearDownCluster() throws Exception { | ||
BookKeeperClusterTestSuite.tearDownCluster(); | ||
} | ||
|
||
private static String getZooKeeperConnectString() throws Exception { | ||
String[] serviceHosts = metadataStore.getServiceUri().getServiceHosts(); | ||
return StringUtils.join(serviceHosts, ','); | ||
} | ||
|
||
private static ZooKeeper createZooKeeper() throws Exception { | ||
return ZooKeeperClient.newBuilder() | ||
.connectString(getZooKeeperConnectString()) | ||
.connectRetryPolicy( | ||
new BoundExponentialBackoffRetryPolicy(1, 10, 100)) | ||
.operationRetryPolicy( | ||
new BoundExponentialBackoffRetryPolicy(1, 10, 100)) | ||
.sessionTimeoutMs(60000) | ||
.build(); | ||
} | ||
|
||
private ZooKeeper zk; | ||
private ZkLedgerUnderreplicationManager urMgr; | ||
|
||
@Before | ||
public void setUp() throws Exception { | ||
this.zk = createZooKeeper(); | ||
ServerConfiguration conf = new ServerConfiguration(baseServerConf); | ||
conf.setStoreSystemTimeAsLedgerUnderreplicatedMarkTime(true); | ||
this.urMgr = new ZkLedgerUnderreplicationManager(conf, zk); | ||
} | ||
|
||
@After | ||
public void tearDown() throws Exception { | ||
if (null != urMgr) { | ||
this.urMgr.close(); | ||
} | ||
if (null != zk) { | ||
this.zk.close(); | ||
} | ||
} | ||
|
||
/** | ||
* Test basic operation on {@link ZkLedgerUnderreplicationManager#markLedgerUnderreplicatedAsync(long, Collection)}. | ||
*/ | ||
@Test | ||
public void testMarkLedgerUnderreplicatedBasic() throws Exception { | ||
long ledgerId = 0xabcdef; | ||
Collection<String> missingBookies = Lists.newArrayList("bookie-1"); | ||
|
||
long prevCtime = -1L; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here probably you can assign current system time to prevCtime, instead of -1 |
||
|
||
// mark when it hasn't been marked before | ||
FutureUtils.result(urMgr.markLedgerUnderreplicatedAsync(ledgerId, missingBookies)); | ||
UnderreplicatedLedgerFormat urLedgerFormat = urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
assertEquals(missingBookies, urLedgerFormat.getReplicaList()); | ||
assertTrue(urLedgerFormat.getCtime() > prevCtime); | ||
prevCtime = urLedgerFormat.getCtime(); | ||
|
||
// mark when it has been marked. but the missing bookies already duplicated there | ||
FutureUtils.result(urMgr.markLedgerUnderreplicatedAsync(ledgerId, missingBookies)); | ||
urLedgerFormat = urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
assertEquals(missingBookies, urLedgerFormat.getReplicaList()); | ||
assertTrue(urLedgerFormat.getCtime() >= prevCtime); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you checking ">=", it should be '==', ctime of marked underreplicated ledger should not change by adding/removing replicaslist to urledger. This would be nice test case to validate that statement. |
||
prevCtime = urLedgerFormat.getCtime(); | ||
|
||
// mark with new bookies when it has been marked | ||
Collection<String> newMissingBookies = Lists.newArrayList("bookie-2", "bookie-3"); | ||
FutureUtils.result(urMgr.markLedgerUnderreplicatedAsync(ledgerId, newMissingBookies)); | ||
urLedgerFormat = urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
assertEquals( | ||
Lists.newArrayList("bookie-1", "bookie-2", "bookie-3"), | ||
urLedgerFormat.getReplicaList() | ||
); | ||
assertTrue(urLedgerFormat.getCtime() >= prevCtime); | ||
} | ||
|
||
/** | ||
* Test {@link ZkLedgerUnderreplicationManager#markLedgerUnderreplicatedAsync(long, Collection)} handles corrupted | ||
* data. | ||
*/ | ||
@Test | ||
public void testMarkLedgerWithCorruptedDataExists() throws Exception { | ||
long ledgerId = 0xabcdee; | ||
String ledgerPath = urMgr.getUrLedgerZnode(ledgerId); | ||
ZkUtils.createFullPathOptimistic( | ||
zk, ledgerPath, "junk data".getBytes(UTF_8), ZkUtils.getACLs(baseServerConf), CreateMode.PERSISTENT); | ||
Collection<String> missingBookies = Lists.newArrayList("bookie-1"); | ||
try { | ||
FutureUtils.result(urMgr.markLedgerUnderreplicatedAsync(ledgerId, missingBookies)); | ||
fail("Should fail to mark ledger underreplicated if there is already corrupted data on zookeeper"); | ||
} catch (ReplicationException re) { | ||
assertTrue(re.getMessage().contains("Invalid underreplicated ledger data for ledger " + ledgerPath)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm not sure how valid it is to rely on exception message. Probably getting ReplicationException should be enoungh |
||
} | ||
byte[] data = zk.getData(ledgerPath, null, null); | ||
assertEquals("junk data", new String(data, UTF_8)); | ||
} | ||
|
||
@Test | ||
public void testMarkLedgerUnderreplicatedConcurrently() throws Exception { | ||
final int numLedgers = 20; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: incorrect variable name. supposed to be numofbookies or something |
||
List<CompletableFuture<Void>> futures = Lists.newArrayListWithExpectedSize(numLedgers); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is soon to be deprecated method - https://google.github.io/guava/releases/22.0/api/docs/com/google/common/collect/Lists.html#newArrayListWithExpectedSize-int- There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new ArrayList<>(numLedgers() ? |
||
long ledgerId = 0xabcc00; | ||
Set<String> expectedBookies = Sets.newHashSet(); | ||
for (int i = 0; i < numLedgers; i++) { | ||
futures.add( | ||
urMgr.markLedgerUnderreplicatedAsync( | ||
ledgerId, Lists.newArrayList("bookie-" + i))); | ||
expectedBookies.add("bookie-" + i); | ||
} | ||
FutureUtils.result(FutureUtils.collect(futures)); | ||
|
||
UnderreplicatedLedgerFormat urLedgerFormat = urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
Set<String> actualBookies = Sets.newHashSet(); | ||
actualBookies.addAll(urLedgerFormat.getReplicaList()); | ||
|
||
assertEquals(expectedBookies, actualBookies); | ||
} | ||
|
||
@Test | ||
public void testMarkLedgerUnderreplicatedConcurrentlyWithDifferentClients() throws Exception { | ||
final int numLedgers = 20; | ||
List<ZooKeeper> zks = new ArrayList<>(numLedgers); | ||
List<ZkLedgerUnderreplicationManager> urMgrs = new ArrayList<>(numLedgers); | ||
|
||
for (int i = 0; i < numLedgers; i++) { | ||
zks.add(createZooKeeper()); | ||
urMgrs.add(new ZkLedgerUnderreplicationManager(baseServerConf, zks.get(i))); | ||
} | ||
|
||
List<CompletableFuture<Void>> futures = Lists.newArrayListWithExpectedSize(numLedgers); | ||
long ledgerId = 0xabcd00; | ||
Set<String> expectedBookies = Sets.newHashSet(); | ||
for (int i = 0; i < numLedgers; i++) { | ||
futures.add( | ||
urMgrs.get(i).markLedgerUnderreplicatedAsync( | ||
ledgerId, Lists.newArrayList("bookie-" + i))); | ||
expectedBookies.add("bookie-" + i); | ||
} | ||
|
||
FutureUtils.result(FutureUtils.collect(futures)); | ||
|
||
UnderreplicatedLedgerFormat urLedgerFormat = urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
Set<String> actualBookies = Sets.newHashSet(); | ||
actualBookies.addAll(urLedgerFormat.getReplicaList()); | ||
|
||
assertEquals(expectedBookies, actualBookies); | ||
|
||
for (LedgerUnderreplicationManager urMgr : urMgrs) { | ||
urMgr.close(); | ||
} | ||
for (ZooKeeper zk : zks) { | ||
zk.close(); | ||
} | ||
} | ||
|
||
|
||
@Test | ||
public void testMarkLedgerUnderreplicatedWhenSessionExpired() throws Exception { | ||
final long ledgerId = 0xabbd00; | ||
@Cleanup | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is much better than having nested try-with-resources blocks |
||
ZooKeeper zk = new ZooKeeper(getZooKeeperConnectString(), 60000, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is instance variable with 'zk' name, better name it differently |
||
@Cleanup | ||
LedgerUnderreplicationManager urMgr = new ZkLedgerUnderreplicationManager(baseServerConf, zk); | ||
// open another zookeeper client to expire current session | ||
@Cleanup | ||
ZooKeeper newZk = new ZooKeeper( | ||
getZooKeeperConnectString(), 60000, null, zk.getSessionId(), zk.getSessionPasswd()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm..forgot the reason, for why would it cause the existing zkc session to expire |
||
try { | ||
FutureUtils.result(urMgr.markLedgerUnderreplicatedAsync( | ||
ledgerId, Lists.newArrayList("bookie-1"))); | ||
fail("Should fail if encountered zookeeper exceptions"); | ||
} catch (KeeperException ke) { | ||
// expected | ||
} | ||
try { | ||
UnderreplicatedLedgerFormat urLedgerFormat = | ||
ZkLedgerUnderreplicationManagerTest.this.urMgr.getLedgerUnreplicationInfo(ledgerId); | ||
fail("The ledger shouldn't been marked as underreplicated"); | ||
} catch (NoNodeException nee) { | ||
// expected | ||
} | ||
} | ||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see that BookKeeperClusterTestSuite has static method with same signature/annotation. In java there is nothing like static method overriding, but this is junit quirk - "in JUnit BeforeClass methods should be named differently, otherwise will result in only the subclass method being run because the parent will be shadowed."
For a regular user it is little confusing to see method with same name and @BeforeClass annotation, probably we should remove @BeforeClass annotation for setUpCluster method in BookKeeperClusterTestSuite or may be pass numBookies as argument to the constructor just like in BookKeeperClusterTestCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also in the new testsuite, if the user named the BeforeClass method little differently (knowingly and unknowingly) then both the BeforeClass methods would be executed with the risk of setting up 2 clusters.