You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by GitBox <gi...@apache.org> on 2022/11/16 13:55:26 UTC
[GitHub] [fluo] keith-turner opened a new pull request, #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
keith-turner opened a new pull request, #1123:
URL: https://github.com/apache/fluo/pull/1123
There was a bug where if a column family was empty and the qual was not empty this would cause lock recovery to fail. The underlying cause was a bug in the Column class. This class has an isFamilySet() method that was returning false when the family was set to the empty string. This cause caused lock recovery code to create an incorrect range.
The Column class was relying on internal behavior of the Bytes class that probably changed and caused this bug.
This commit adds a new IT that recreates this bug. If the new IT is run w/o the fix to the Column class then it would fail as follows.
```
Running org.apache.fluo.integration.impl.FailureIT
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 8.011 sec <<< FAILURE! - in org.apache.fluo.integration.impl.FailureIT
testRecoverEmptyColumn(org.apache.fluo.integration.impl.FailureIT) Time elapsed: 7.096 sec <<< ERROR!
java.lang.IllegalStateException: can not abort : bob bal 5 (UNKNOWN)
at org.apache.fluo.integration.impl.FailureIT.testRecoverEmptyColumn(FailureIT.java:688)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] wjsl commented on a diff in pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
wjsl commented on code in PR #1123:
URL: https://github.com/apache/fluo/pull/1123#discussion_r1024081999
##########
modules/api/src/main/java/org/apache/fluo/api/data/Column.java:
##########
@@ -28,26 +28,42 @@
public final class Column implements Comparable<Column>, Serializable {
private static final long serialVersionUID = 1L;
- private static final Bytes UNSET = Bytes.of(new byte[0]);
- private Bytes family = UNSET;
- private Bytes qualifier = UNSET;
- private Bytes visibility = UNSET;
+ private final Bytes family;
+ private final Bytes qualifier;
+ private final Bytes visibility;
+
+ private final boolean isFamilySet;
+ private final boolean isQualifierSet;
+ private final boolean isVisibilitySet;
Review Comment:
Not to spread too much FUD, but `Optional` isn't necessarily free. There's some size overhead in having another Object member, as well as access overhead due to need a function call to either extract the value, or build a pipeline via `map`/`flatMap`. I'd be curious if the impact were greater with `ColumnVisibiliy`s enabled in my crusty ol' draft PR, since applications could be on the hook to create more `Column` objects based on labels.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1123:
URL: https://github.com/apache/fluo/pull/1123#discussion_r1024089177
##########
modules/api/src/main/java/org/apache/fluo/api/data/Column.java:
##########
@@ -28,26 +28,42 @@
public final class Column implements Comparable<Column>, Serializable {
private static final long serialVersionUID = 1L;
- private static final Bytes UNSET = Bytes.of(new byte[0]);
- private Bytes family = UNSET;
- private Bytes qualifier = UNSET;
- private Bytes visibility = UNSET;
+ private final Bytes family;
+ private final Bytes qualifier;
+ private final Bytes visibility;
+
+ private final boolean isFamilySet;
+ private final boolean isQualifierSet;
+ private final boolean isVisibilitySet;
Review Comment:
I considered using Optional but decided against it because it would add more Objects to GC and another level of indirection (so much pointer chasing in java in general, I suspect this why even java compiles to native its still slower than C++). This low level data class already has a good bit of both (objs and indirection), didn't want to add more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] ctubbsii commented on a diff in pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #1123:
URL: https://github.com/apache/fluo/pull/1123#discussion_r1024511529
##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/impl/FailureIT.java:
##########
@@ -652,4 +655,40 @@ private boolean wasRolledBackPrimary(long startTs, String rolledBackRow)
return sawExpected;
}
+ /*
+ * There was a bug where a locked column with an empty family could not be recovered.
+ */
+ @Test
+ public void testRecoverEmptyColumn() {
+ Column ecol = new Column("", "bal");
+
+ TestTransaction tx1 = new TestTransaction(env);
+
+ tx1.set("bob", ecol, "10");
+ tx1.set("joe", ecol, "20");
+ tx1.set("jill", ecol, "60");
+
+ tx1.done();
+
+ // partially commit a transaction, leaving the row 'joe' with a lock.
+ TestTransaction tx2 = new TestTransaction(env);
+ TestUtil.increment(tx2, "bob", ecol, 5);
+ TestUtil.increment(tx2, "joe", ecol, -5);
+
+ CommitData cd = tx2.createCommitData();
+ RowColumn primary = new RowColumn("bob", ecol);
+ Assert.assertTrue(tx2.preCommit(cd, primary));
+ Stamp commitTs = env.getSharedResources().getOracleClient().getStamp();
+ tx2.commitPrimaryColumn(cd, commitTs);
+ tx2.close();
+
+ // this transaction should roll forward the above transaction
+ TestTransaction tx3 = new TestTransaction(env);
+ Assert.assertEquals("15", tx3.gets("bob", ecol));
+ Assert.assertEquals("15", tx3.gets("joe", ecol));
+ Assert.assertEquals("60", tx3.gets("jill", ecol));
+ tx3.close();
Review Comment:
I took a look, and it's weird that it doesn't. It should. I created #1124 to address it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner merged pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
keith-turner merged PR #1123:
URL: https://github.com/apache/fluo/pull/1123
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] ctubbsii commented on a diff in pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #1123:
URL: https://github.com/apache/fluo/pull/1123#discussion_r1024056223
##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/impl/FailureIT.java:
##########
@@ -652,4 +655,40 @@ private boolean wasRolledBackPrimary(long startTs, String rolledBackRow)
return sawExpected;
}
+ /*
+ * There was a bug where a locked column with an empty family could not be recovered.
+ */
+ @Test
+ public void testRecoverEmptyColumn() {
+ Column ecol = new Column("", "bal");
+
+ TestTransaction tx1 = new TestTransaction(env);
+
+ tx1.set("bob", ecol, "10");
+ tx1.set("joe", ecol, "20");
+ tx1.set("jill", ecol, "60");
+
+ tx1.done();
+
+ // partially commit a transaction, leaving the row 'joe' with a lock.
+ TestTransaction tx2 = new TestTransaction(env);
+ TestUtil.increment(tx2, "bob", ecol, 5);
+ TestUtil.increment(tx2, "joe", ecol, -5);
+
+ CommitData cd = tx2.createCommitData();
+ RowColumn primary = new RowColumn("bob", ecol);
+ Assert.assertTrue(tx2.preCommit(cd, primary));
+ Stamp commitTs = env.getSharedResources().getOracleClient().getStamp();
+ tx2.commitPrimaryColumn(cd, commitTs);
+ tx2.close();
+
+ // this transaction should roll forward the above transaction
+ TestTransaction tx3 = new TestTransaction(env);
+ Assert.assertEquals("15", tx3.gets("bob", ecol));
+ Assert.assertEquals("15", tx3.gets("joe", ecol));
+ Assert.assertEquals("60", tx3.gets("jill", ecol));
+ tx3.close();
Review Comment:
Tests could use try with resources to close the transactions
##########
modules/api/src/main/java/org/apache/fluo/api/data/Column.java:
##########
@@ -28,26 +28,42 @@
public final class Column implements Comparable<Column>, Serializable {
private static final long serialVersionUID = 1L;
- private static final Bytes UNSET = Bytes.of(new byte[0]);
- private Bytes family = UNSET;
- private Bytes qualifier = UNSET;
- private Bytes visibility = UNSET;
+ private final Bytes family;
+ private final Bytes qualifier;
+ private final Bytes visibility;
+
+ private final boolean isFamilySet;
+ private final boolean isQualifierSet;
+ private final boolean isVisibilitySet;
Review Comment:
Instead of 6 fields, could use Optional to have only 3. Could use Optional.orElse(emptyBytes) in the getter instead of checking if the corresponding boolean is set
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #1123:
URL: https://github.com/apache/fluo/pull/1123#issuecomment-1317079540
All ITs pass for ee51445
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1123: Fixes bug resolving lock w/ empty col fam. Reported in #660
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1123:
URL: https://github.com/apache/fluo/pull/1123#discussion_r1024090838
##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/impl/FailureIT.java:
##########
@@ -652,4 +655,40 @@ private boolean wasRolledBackPrimary(long startTs, String rolledBackRow)
return sawExpected;
}
+ /*
+ * There was a bug where a locked column with an empty family could not be recovered.
+ */
+ @Test
+ public void testRecoverEmptyColumn() {
+ Column ecol = new Column("", "bal");
+
+ TestTransaction tx1 = new TestTransaction(env);
+
+ tx1.set("bob", ecol, "10");
+ tx1.set("joe", ecol, "20");
+ tx1.set("jill", ecol, "60");
+
+ tx1.done();
+
+ // partially commit a transaction, leaving the row 'joe' with a lock.
+ TestTransaction tx2 = new TestTransaction(env);
+ TestUtil.increment(tx2, "bob", ecol, 5);
+ TestUtil.increment(tx2, "joe", ecol, -5);
+
+ CommitData cd = tx2.createCommitData();
+ RowColumn primary = new RowColumn("bob", ecol);
+ Assert.assertTrue(tx2.preCommit(cd, primary));
+ Stamp commitTs = env.getSharedResources().getOracleClient().getStamp();
+ tx2.commitPrimaryColumn(cd, commitTs);
+ tx2.close();
+
+ // this transaction should roll forward the above transaction
+ TestTransaction tx3 = new TestTransaction(env);
+ Assert.assertEquals("15", tx3.gets("bob", ecol));
+ Assert.assertEquals("15", tx3.gets("joe", ecol));
+ Assert.assertEquals("60", tx3.gets("jill", ecol));
+ tx3.close();
Review Comment:
Gave this a try. Seems the TestTransaction type used in the ITs does not implement AutoCloseable so it did not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org