Skip to content

Commit

Permalink
ISSUE #726: Unit Tests failure with BKMetadataVersionException
Browse files Browse the repository at this point in the history 
Descriptions of the changes in this PR:

Problem:

The problem is introduced after making storing ctime optional. the verification become inconsistent because the way how ctime was assigned and compared. It causes the test cases failing in #726

Solution:

This change is to change `isConflict` to ignore comparing ctime if client disables storing system time as ctime.

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>

This closes #730 from sijie/fix_ctime_issue, closes #726
  • Loading branch information
@sijie @jiazhai
sijie authored and jiazhai committed Nov 16, 2017
1 parent e253ca3 commit c9a150b
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 9 deletions.
35 changes: 30 additions & 5 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,13 @@ public LedgerMetadata(int ensembleSize,
this.ensembleSize = ensembleSize;
this.writeQuorumSize = writeQuorumSize;
this.ackQuorumSize = ackQuorumSize;
this.ctime = System.currentTimeMillis();
if (storeSystemtimeAsLedgerCreationTime) {
this.ctime = System.currentTimeMillis();
} else {
// if client disables storing its system time as ledger creation time, there should be no ctime at this
// moment.
this.ctime = -1L;
}
this.storeSystemtimeAsLedgerCreationTime = storeSystemtimeAsLedgerCreationTime;

/*
Expand All @@ -119,6 +125,10 @@ public LedgerMetadata(int ensembleSize,
}
}

/**
* Used for testing purpose only.
*/
@VisibleForTesting
public LedgerMetadata(int ensembleSize, int writeQuorumSize, int ackQuorumSize,
BookKeeper.DigestType digestType, byte[] password) {
this(ensembleSize, writeQuorumSize, ackQuorumSize, digestType, password, null, false);
Expand All @@ -130,7 +140,6 @@ public LedgerMetadata(int ensembleSize, int writeQuorumSize, int ackQuorumSize,
LedgerMetadata(LedgerMetadata other) {
this.ensembleSize = other.ensembleSize;
this.writeQuorumSize = other.writeQuorumSize;
this.ctime = other.ctime;
this.ackQuorumSize = other.ackQuorumSize;
this.length = other.length;
this.lastEntryId = other.lastEntryId;
Expand All @@ -139,6 +148,8 @@ public LedgerMetadata(int ensembleSize, int writeQuorumSize, int ackQuorumSize,
this.version = other.version;
this.hasPassword = other.hasPassword;
this.digestType = other.digestType;
this.ctime = other.ctime;
this.storeSystemtimeAsLedgerCreationTime = other.storeSystemtimeAsLedgerCreationTime;
this.password = new byte[other.password.length];
System.arraycopy(other.password, 0, this.password, 0, other.password.length);
// copy the ensembles
Expand Down Expand Up @@ -178,6 +189,10 @@ public int getWriteQuorumSize() {
return writeQuorumSize;
}

public int getAckQuorumSize() {
return ackQuorumSize;
}

/**
* Get the creation timestamp of the ledger
* @return
Expand All @@ -186,8 +201,9 @@ public long getCtime() {
return ctime;
}

public int getAckQuorumSize() {
return ackQuorumSize;
@VisibleForTesting
void setCtime(long ctime) {
this.ctime = ctime;
}

/**
Expand Down Expand Up @@ -430,8 +446,10 @@ public static LedgerMetadata parseConfig(byte[] bytes, Version version, Optional
lc.writeQuorumSize = data.getQuorumSize();
if (data.hasCtime()) {
lc.ctime = data.getCtime();
lc.storeSystemtimeAsLedgerCreationTime = true;
} else if (msCtime.isPresent()) {
lc.ctime = msCtime.get();
lc.storeSystemtimeAsLedgerCreationTime = false;
}
if (data.hasAckQuorumSize()) {
lc.ackQuorumSize = data.getAckQuorumSize();
Expand Down Expand Up @@ -583,7 +601,6 @@ boolean isConflictWith(LedgerMetadata newMeta) {
if (metadataFormatVersion != newMeta.metadataFormatVersion ||
ensembleSize != newMeta.ensembleSize ||
writeQuorumSize != newMeta.writeQuorumSize ||
ctime != newMeta.ctime ||
ackQuorumSize != newMeta.ackQuorumSize ||
length != newMeta.length ||
state != newMeta.state ||
Expand All @@ -592,6 +609,14 @@ boolean isConflictWith(LedgerMetadata newMeta) {
!LedgerMetadata.areByteArrayValMapsEqual(customMetadata, newMeta.customMetadata)) {
return true;
}

// verify the ctime
if (storeSystemtimeAsLedgerCreationTime != newMeta.storeSystemtimeAsLedgerCreationTime) {
return true;
} else if (storeSystemtimeAsLedgerCreationTime) {
return ctime != newMeta.ctime;
}

if (state == LedgerMetadataFormat.State.CLOSED
&& lastEntryId != newMeta.lastEntryId) {
return true;
Expand Down
62 changes: 58 additions & 4 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerMetadataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@
*/
public class LedgerMetadataTest {

private static final byte[] passwd = "testPasswd".getBytes(UTF_8);

@Test
public void testStoreSystemtimeAsLedgerCtimeEnabled()
throws Exception {
byte[] passwd = "testPasswd".getBytes(UTF_8);

LedgerMetadata lm = new LedgerMetadata(
3,
3,
Expand All @@ -53,8 +53,6 @@ public void testStoreSystemtimeAsLedgerCtimeEnabled()
@Test
public void testStoreSystemtimeAsLedgerCtimeDisabled()
throws Exception {
byte[] passwd = "testPasswd".getBytes(UTF_8);

LedgerMetadata lm = new LedgerMetadata(
3,
3,
Expand All @@ -67,4 +65,60 @@ public void testStoreSystemtimeAsLedgerCtimeDisabled()
assertFalse(format.hasCtime());
}

@Test
public void testIsConflictWithStoreSystemtimeAsLedgerCtimeDisabled() {
LedgerMetadata lm1 = new LedgerMetadata(
3,
3,
2,
DigestType.CRC32,
passwd,
Collections.emptyMap(),
false);
LedgerMetadata lm2 = new LedgerMetadata(lm1);

lm1.setCtime(1L);
lm2.setCtime(2L);
assertFalse(lm1.isConflictWith(lm2));
}

@Test
public void testIsConflictWithStoreSystemtimeAsLedgerCtimeEnabled() {
LedgerMetadata lm1 = new LedgerMetadata(
3,
3,
2,
DigestType.CRC32,
passwd,
Collections.emptyMap(),
true);
LedgerMetadata lm2 = new LedgerMetadata(lm1);

lm1.setCtime(1L);
lm2.setCtime(2L);
assertTrue(lm1.isConflictWith(lm2));
}

@Test
public void testIsConflictWithDifferentStoreSystemtimeAsLedgerCtimeFlags() {
LedgerMetadata lm1 = new LedgerMetadata(
3,
3,
2,
DigestType.CRC32,
passwd,
Collections.emptyMap(),
true);
LedgerMetadata lm2 = new LedgerMetadata(
3,
3,
2,
DigestType.CRC32,
passwd,
Collections.emptyMap(),
false);

assertTrue(lm1.isConflictWith(lm2));
}

}

0 comments on commit c9a150b

Please sign in to comment.