You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/03/18 00:52:41 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3245: add delay after setting prop to improve test reliability
EdColeman opened a new pull request, #3245:
URL: https://github.com/apache/accumulo/pull/3245
* Add delay to exporttableImporttable after setting property.
The shell server IT exporttableImporttable test occasionally fails - add delay after setting property to allow time for ZooKeeper propagation.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1146651160
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java:
##########
@@ -138,16 +138,20 @@ private class DeleteWatcher implements PropChangeListener {
@Override
public void zkChangeEvent(PropStoreKey<?> propStoreKey) {
- // no-op. changes handled by prop store impl
+ clearLocalOnEvent(propStoreKey);
}
@Override
public void cacheChangeEvent(PropStoreKey<?> propStoreKey) {
- // no-op. changes handled by prop store impl
+ clearLocalOnEvent(propStoreKey);
}
@Override
public void deleteEvent(PropStoreKey<?> propStoreKey) {
+ clearLocalOnEvent(propStoreKey);
+ }
+
+ private void clearLocalOnEvent(PropStoreKey<?> propStoreKey) {
if (propStoreKey instanceof NamespacePropKey) {
log.trace("configuration snapshot refresh: Handle namespace delete for {}", propStoreKey);
Review Comment:
Addressed in 455558d458
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1144843530
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -167,63 +167,94 @@ public void exporttableImporttable() throws Exception {
getCluster().createAccumuloClient(getPrincipal(), new PasswordToken(getRootPassword()))) {
client.securityOperations().grantNamespacePermission(getPrincipal(), "",
NamespacePermission.ALTER_NAMESPACE);
+
+ final String table = getUniqueNames(1)[0] + "_export_src";
+ final String table2 = table + "_import_tgt";
Review Comment:
Not a big deal (also might be intentional) but wouldn't table2 have `_export_src_import_tgt` at the end of it since it appends to `table`? This shouldn't hurt the test but might be a confusing table name if we ever need to debug using these names.
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -474,35 +512,23 @@ public void setIterOptionPrompt() throws Exception {
// Name on the CLI should override OptionDescriber (or user input name, in this case)
ts.exec("setiter -scan -class " + COLUMN_FAMILY_COUNTER_ITERATOR + " -p 30 -name cfcounter",
true);
- expectedKey = "table.iterator.scan.cfcounter";
- expectedValue = "30," + COLUMN_FAMILY_COUNTER_ITERATOR;
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name1";
- expectedValue = "value1.1,value1.2,value1.3";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name2";
- expectedValue = "value2";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- ts.exec("deletetable " + tableName3, true);
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter").equals("30," + COLUMN_FAMILY_COUNTER_ITERATOR),
+ 5000, 500));
- }
- }
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter.opt.name1").equals("value1.1,value1.2,value1.3"),
+ 5000, 500));
- protected void checkTableForProperty(TableOperations tops, String tableName, String expectedKey,
Review Comment:
An alternative to removing this method could be to move the `assertTrue(Wait.waitFor())` blocks that were added in this PR into this method. Not sure if it would be better but if that were to happen then I think a lot of this code would not need to be changed and the logic could be reused.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1144888481
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -167,63 +167,94 @@ public void exporttableImporttable() throws Exception {
getCluster().createAccumuloClient(getPrincipal(), new PasswordToken(getRootPassword()))) {
client.securityOperations().grantNamespacePermission(getPrincipal(), "",
NamespacePermission.ALTER_NAMESPACE);
+
+ final String table = getUniqueNames(1)[0] + "_export_src";
+ final String table2 = table + "_import_tgt";
Review Comment:
fixed in 32ef064f89
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1146445619
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java:
##########
@@ -138,16 +138,20 @@ private class DeleteWatcher implements PropChangeListener {
@Override
public void zkChangeEvent(PropStoreKey<?> propStoreKey) {
- // no-op. changes handled by prop store impl
+ clearLocalOnEvent(propStoreKey);
}
@Override
public void cacheChangeEvent(PropStoreKey<?> propStoreKey) {
- // no-op. changes handled by prop store impl
+ clearLocalOnEvent(propStoreKey);
}
@Override
public void deleteEvent(PropStoreKey<?> propStoreKey) {
+ clearLocalOnEvent(propStoreKey);
+ }
+
+ private void clearLocalOnEvent(PropStoreKey<?> propStoreKey) {
if (propStoreKey instanceof NamespacePropKey) {
log.trace("configuration snapshot refresh: Handle namespace delete for {}", propStoreKey);
Review Comment:
This log message still says that it's handling delete, but the message applies to other events, too now.
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -474,35 +512,23 @@ public void setIterOptionPrompt() throws Exception {
// Name on the CLI should override OptionDescriber (or user input name, in this case)
ts.exec("setiter -scan -class " + COLUMN_FAMILY_COUNTER_ITERATOR + " -p 30 -name cfcounter",
true);
- expectedKey = "table.iterator.scan.cfcounter";
- expectedValue = "30," + COLUMN_FAMILY_COUNTER_ITERATOR;
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name1";
- expectedValue = "value1.1,value1.2,value1.3";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name2";
- expectedValue = "value2";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- ts.exec("deletetable " + tableName3, true);
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter").equals("30," + COLUMN_FAMILY_COUNTER_ITERATOR),
+ 5000, 500));
- }
- }
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter.opt.name1").equals("value1.1,value1.2,value1.3"),
+ 5000, 500));
- protected void checkTableForProperty(TableOperations tops, String tableName, String expectedKey,
Review Comment:
I think it's okay to abstract this boilerplate stuff in the method. It helps us implement the check consistently. We might forget to use this pattern elsewhere... but if it's only done in one place, that makes it easier to do the check the same way each time... so long as our requirements are the same.
--
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
[GitHub] [accumulo] EdColeman commented on pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#issuecomment-1481700076
455558d458 - added back the check table property metod
--
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
[GitHub] [accumulo] EdColeman commented on pull request #3245: add delay after setting prop to improve test reliability
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#issuecomment-1477082239
This change does not resolve the race condition - additional code changes may be required - looking into it now. With either add to this PR, or close and submit a new one.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1144893643
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -474,35 +512,23 @@ public void setIterOptionPrompt() throws Exception {
// Name on the CLI should override OptionDescriber (or user input name, in this case)
ts.exec("setiter -scan -class " + COLUMN_FAMILY_COUNTER_ITERATOR + " -p 30 -name cfcounter",
true);
- expectedKey = "table.iterator.scan.cfcounter";
- expectedValue = "30," + COLUMN_FAMILY_COUNTER_ITERATOR;
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name1";
- expectedValue = "value1.1,value1.2,value1.3";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name2";
- expectedValue = "value2";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- ts.exec("deletetable " + tableName3, true);
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter").equals("30," + COLUMN_FAMILY_COUNTER_ITERATOR),
+ 5000, 500));
- }
- }
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter.opt.name1").equals("value1.1,value1.2,value1.3"),
+ 5000, 500));
- protected void checkTableForProperty(TableOperations tops, String tableName, String expectedKey,
Review Comment:
It seemed better to favor explicitly calling `Wait.waitFor()` because that made it clear there in the test what was happening - wrapping it in checkForTableProperty() was unique to this test and `Wait.waitFor()` more common elsewhere - but either way is fine if reducing the number of changes is more important for this change for this test.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#discussion_r1146650673
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java:
##########
@@ -474,35 +512,23 @@ public void setIterOptionPrompt() throws Exception {
// Name on the CLI should override OptionDescriber (or user input name, in this case)
ts.exec("setiter -scan -class " + COLUMN_FAMILY_COUNTER_ITERATOR + " -p 30 -name cfcounter",
true);
- expectedKey = "table.iterator.scan.cfcounter";
- expectedValue = "30," + COLUMN_FAMILY_COUNTER_ITERATOR;
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name1";
- expectedValue = "value1.1,value1.2,value1.3";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- expectedKey = "table.iterator.scan.cfcounter.opt.name2";
- expectedValue = "value2";
- checkTableForProperty(tops, tableName3, expectedKey, expectedValue);
- ts.exec("deletetable " + tableName3, true);
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter").equals("30," + COLUMN_FAMILY_COUNTER_ITERATOR),
+ 5000, 500));
- }
- }
+ assertTrue(Wait.waitFor(
+ () -> client.tableOperations().getConfiguration(tableName3)
+ .get("table.iterator.scan.cfcounter.opt.name1").equals("value1.1,value1.2,value1.3"),
+ 5000, 500));
- protected void checkTableForProperty(TableOperations tops, String tableName, String expectedKey,
Review Comment:
Addressed in 455558d458
--
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
[GitHub] [accumulo] cshannon commented on pull request #3245: add delay after setting prop to improve test reliability
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#issuecomment-1474841850
Adding the sleep is simple but picking the right amount is pretty arbitrary. Can you use the [Wait](https://github.com/apache/accumulo/blob/540179d1f52dcc478eee3a3ee3c5fac106736c8b/test/src/main/java/org/apache/accumulo/test/util/Wait.java) util class and just wait until the property has propagated to ZK before continuing?
I used it quite a few times inside of PropStoreConfigIT to wait for propagation of properties. For example: https://github.com/apache/accumulo/blob/540179d1f52dcc478eee3a3ee3c5fac106736c8b/test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT.java#L95-L124
--
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
[GitHub] [accumulo] EdColeman commented on pull request #3245: add delay after setting prop to improve test reliability
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#issuecomment-1476283642
The change 5872a846d2 has a large diff because of formatting changes due to changing try-resource client scope.
Also modified the property check sleep to shorten initial time and add a back-off.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3245:
URL: https://github.com/apache/accumulo/pull/3245#issuecomment-1482087303
I think this fixes #3248
--
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
[GitHub] [accumulo] ctubbsii merged pull request #3245: Clear ServerConfigurationFactory cache on ZooKeeper notifications. Improve ShellServerIT test.
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3245:
URL: https://github.com/apache/accumulo/pull/3245
--
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