You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2023/11/13 16:44:06 UTC
[PR] Add support for checking WALs in condition mutations [accumulo]
DomGarguilo opened a new pull request, #3948:
URL: https://github.com/apache/accumulo/pull/3948
Closes #3847
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #3948:
URL: https://github.com/apache/accumulo/pull/3948#issuecomment-1814710601
Working on incorporating review suggestions currently.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3948:
URL: https://github.com/apache/accumulo/pull/3948#discussion_r1394641785
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -321,6 +325,59 @@ public void testFiles() throws Exception {
}
}
+ @Test
+ public void testWALs() {
+ var context = cluster.getServerContext();
+
+ // Test adding a WAL to a tablet and verifying its presence
+ String walFilePath =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry originalLogEntry = new LogEntry(walFilePath);
+ ConditionalTabletsMutatorImpl ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(originalLogEntry).submit(tm -> false);
+ var results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+ assertEquals(List.of(originalLogEntry), context.getAmple().readTablet(e1).getLogs(),
+ "The original LogEntry should be present.");
+
+ // Test adding another WAL and verifying the update
+ String walFilePath2 =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry newLogEntry = new LogEntry(walFilePath2);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(newLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // Verify that both the original and new WALs are present
+ Set<LogEntry> expectedLogs = Set.of(originalLogEntry, newLogEntry);
+ HashSet<LogEntry> actualLogs = new HashSet<>(context.getAmple().readTablet(e1).getLogs());
+ assertEquals(expectedLogs, actualLogs, "Both original and new LogEntry should be present.");
+
+ // Test that requireSame with just one of the two WALs fails
+ TabletMetadata tm1 = TabletMetadata.builder(e1).putWal(originalLogEntry).build(LOGS);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(tm1, LOGS)
+ .deleteWal(originalLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.REJECTED, results.get(e1).getStatus());
Review Comment:
May be able to use Guavas Sets.powerSets methods to create a cleaner implementation than what I posted above, not sure. Could call `Sets.powerSet(original, new, and other walog)` and iterate over what it returns ignoring the one set that would be accepted.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #3948:
URL: https://github.com/apache/accumulo/pull/3948#issuecomment-1814859286
a3e4721 should incorporate the suggestion made.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #3948:
URL: https://github.com/apache/accumulo/pull/3948
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3948:
URL: https://github.com/apache/accumulo/pull/3948#discussion_r1394641785
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -321,6 +325,59 @@ public void testFiles() throws Exception {
}
}
+ @Test
+ public void testWALs() {
+ var context = cluster.getServerContext();
+
+ // Test adding a WAL to a tablet and verifying its presence
+ String walFilePath =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry originalLogEntry = new LogEntry(walFilePath);
+ ConditionalTabletsMutatorImpl ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(originalLogEntry).submit(tm -> false);
+ var results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+ assertEquals(List.of(originalLogEntry), context.getAmple().readTablet(e1).getLogs(),
+ "The original LogEntry should be present.");
+
+ // Test adding another WAL and verifying the update
+ String walFilePath2 =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry newLogEntry = new LogEntry(walFilePath2);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(newLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // Verify that both the original and new WALs are present
+ Set<LogEntry> expectedLogs = Set.of(originalLogEntry, newLogEntry);
+ HashSet<LogEntry> actualLogs = new HashSet<>(context.getAmple().readTablet(e1).getLogs());
+ assertEquals(expectedLogs, actualLogs, "Both original and new LogEntry should be present.");
+
+ // Test that requireSame with just one of the two WALs fails
+ TabletMetadata tm1 = TabletMetadata.builder(e1).putWal(originalLogEntry).build(LOGS);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(tm1, LOGS)
+ .deleteWal(originalLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.REJECTED, results.get(e1).getStatus());
Review Comment:
May be able to use Guavas Sets.powerSets methods to create a clean implementation than what I posted above, not sure. Could call `Sets.powerSet(original, new, and other walog)` and iterate over what it returns ignoring the one set that would be accepted.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for checking WALs in condition mutations [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3948:
URL: https://github.com/apache/accumulo/pull/3948#discussion_r1394614820
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -321,6 +325,59 @@ public void testFiles() throws Exception {
}
}
+ @Test
+ public void testWALs() {
+ var context = cluster.getServerContext();
+
+ // Test adding a WAL to a tablet and verifying its presence
+ String walFilePath =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry originalLogEntry = new LogEntry(walFilePath);
+ ConditionalTabletsMutatorImpl ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(originalLogEntry).submit(tm -> false);
+ var results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+ assertEquals(List.of(originalLogEntry), context.getAmple().readTablet(e1).getLogs(),
+ "The original LogEntry should be present.");
+
+ // Test adding another WAL and verifying the update
+ String walFilePath2 =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry newLogEntry = new LogEntry(walFilePath2);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(newLogEntry).submit(tm -> false);
Review Comment:
Can test the empty set here.
```suggestion
// create a tablet metadata with no write ahead logs
var tmEmptySet = TabletMetadata.builder(e1).build(LOGS);
ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(LOGS, tmEmptySet).putWal(newLogEntry).submit(tm -> false);
```
##########
test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java:
##########
@@ -321,6 +325,59 @@ public void testFiles() throws Exception {
}
}
+ @Test
+ public void testWALs() {
+ var context = cluster.getServerContext();
+
+ // Test adding a WAL to a tablet and verifying its presence
+ String walFilePath =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry originalLogEntry = new LogEntry(walFilePath);
+ ConditionalTabletsMutatorImpl ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(originalLogEntry).submit(tm -> false);
+ var results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+ assertEquals(List.of(originalLogEntry), context.getAmple().readTablet(e1).getLogs(),
+ "The original LogEntry should be present.");
+
+ // Test adding another WAL and verifying the update
+ String walFilePath2 =
+ java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
+ LogEntry newLogEntry = new LogEntry(walFilePath2);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().putWal(newLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.ACCEPTED, results.get(e1).getStatus());
+
+ // Verify that both the original and new WALs are present
+ Set<LogEntry> expectedLogs = Set.of(originalLogEntry, newLogEntry);
+ HashSet<LogEntry> actualLogs = new HashSet<>(context.getAmple().readTablet(e1).getLogs());
+ assertEquals(expectedLogs, actualLogs, "Both original and new LogEntry should be present.");
+
+ // Test that requireSame with just one of the two WALs fails
+ TabletMetadata tm1 = TabletMetadata.builder(e1).putWal(originalLogEntry).build(LOGS);
+ ctmi = new ConditionalTabletsMutatorImpl(context);
+ ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(tm1, LOGS)
+ .deleteWal(originalLogEntry).submit(tm -> false);
+ results = ctmi.process();
+ assertEquals(Status.REJECTED, results.get(e1).getStatus());
Review Comment:
For the rejected case it would be good to test a few different set intersection types like superset, subset, empty set, overlapping set, disjoint set, etc. Tried to add the ones I could think of in the suggestion below.
```suggestion
List<TabletMetadata> tabletsWithDifferentLogs = new ArrayList();
tabletsWithDifferentLogs.add(tmEmptySet);
tabletsWithDifferentLogs.add(TabletMetadata.builder(e1).putWal(originalLogEntry).build(LOGS));
tabletsWithDifferentLogs.add(TabletMetadata.builder(e1).putWal(newLogEntry).build(LOGS));
String walFilePath3 =
java.nio.file.Path.of("tserver:8080", UUID.randomUUID().toString()).toString();
LogEntry otherLogEntry = new LogEntry(walFilePath3);
// add tablet that has same logs as current tablet plus an extra log, a superset
tabletsWithDifferentLogs.add(TabletMetadata.builder(e1).putWal(originalLogEntry).putWal(newLogEntry).putWal(otherLogEntry).build(LOGS));
tabletsWithDifferentLogs.add(TabletMetadata.builder(e1).putWal(otherLogEntry).build(LOGS));
tabletsWithDifferentLogs.add(TabletMetadata.builder(e1).putWal(originalLogEntry).putWal(otherLogEntry).build(LOGS));
for(TabletMetadata tm : tabletsWithDifferentLogs) {
TabletMetadata tm1 = TabletMetadata.builder(e1).putWal(originalLogEntry).build(LOGS);
ctmi = new ConditionalTabletsMutatorImpl(context);
ctmi.mutateTablet(e1).requireAbsentOperation().requireSame(tm1, LOGS)
.deleteWal(originalLogEntry).submit(tm -> false);
results = ctmi.process();
assertEquals(Status.REJECTED, results.get(e1).getStatus());
// TODO read tablet and check logs are as expected
}
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org