You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/12/17 19:45:00 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
cshannon opened a new pull request, #3135:
URL: https://github.com/apache/accumulo/pull/3135
This fixes the test that was broken by #3106 by adding back in the master to manager property renamer for just the test. Even though the old master prefixed properties were removed and upgrade isn't needed there are no other renamers that currently exist so adding back in the renamer just for the test will at least verify the functionality still works.
--
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 #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#discussion_r1051492282
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
Review Comment:
Instead of keeping that specific renamer around, can we create a dummy renamer for testing purposes, so it's not tied to any specific previous code that looks like it tech debt?
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
+ private static List<DeprecatedPropertyUtil.PropertyRenamer> renamers;
+
private InstanceId instanceId = null;
@TempDir
private static File tempDir;
+ static {
+ try {
+ // Make the renamers list visible to be modified
+ final Field renamersField = DeprecatedPropertyUtil.class.getDeclaredField("renamers");
+ renamersField.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ List<DeprecatedPropertyUtil.PropertyRenamer> renamers = (List<
+ DeprecatedPropertyUtil.PropertyRenamer>) renamersField.get(DeprecatedPropertyUtil.class);
+ ConfigPropertyUpgraderIT.renamers = renamers;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
Review Comment:
I think we could probably avoid the static initializer stuff and all the type-safety check bypassing if the code we're testing is tweaked slightly to make it easier to test with a method that makes the list visible for testing purposes.
--
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 #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#issuecomment-1358492975
Yeah, I will probably just stick to the browser next time. There's not going to be too many cases when something needs to be urgently merged so waiting for desktop access is fine to make sure it's correct.
--
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 a diff in pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#discussion_r1051623111
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
+ private static List<DeprecatedPropertyUtil.PropertyRenamer> renamers;
+
private InstanceId instanceId = null;
@TempDir
private static File tempDir;
+ static {
+ try {
+ // Make the renamers list visible to be modified
+ final Field renamersField = DeprecatedPropertyUtil.class.getDeclaredField("renamers");
+ renamersField.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ List<DeprecatedPropertyUtil.PropertyRenamer> renamers = (List<
+ DeprecatedPropertyUtil.PropertyRenamer>) renamersField.get(DeprecatedPropertyUtil.class);
+ ConfigPropertyUpgraderIT.renamers = renamers;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
Review Comment:
Nevermind, I'm just going to go with your original idea to add a method to get the list so it's visible as adding another util method doesn't help since the upgrader logic won't ever call 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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#issuecomment-1358494070
I should add that in this case I tried it so the build would stop being broken but as I said, not like waiting another couple hours would matter 😀
--
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 merged pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #3135:
URL: https://github.com/apache/accumulo/pull/3135
--
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 a diff in pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#discussion_r1051620435
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
Review Comment:
Yeah I will rework this a bit in combination with my other comment so it's a bit cleaner and uses a dummy renamer.
--
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 #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#issuecomment-1358486955
I came here to comment about the message not matching the final committed changes, but it looks like you already know :smiley_cat: The app is not intuitive. Last time I tried, I think it did give me the option, but it wasn't obvious. I try to use it very sparingly.
--
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 #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#issuecomment-1356434078
Obviously the other option is to just remove the test assertions that broke but I thought it would be good to still have some tests to verify the renamers will work if we add new ones in the future as currently the list is empty.
--
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 a diff in pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#discussion_r1051620375
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
+ private static List<DeprecatedPropertyUtil.PropertyRenamer> renamers;
+
private InstanceId instanceId = null;
@TempDir
private static File tempDir;
+ static {
+ try {
+ // Make the renamers list visible to be modified
+ final Field renamersField = DeprecatedPropertyUtil.class.getDeclaredField("renamers");
+ renamersField.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ List<DeprecatedPropertyUtil.PropertyRenamer> renamers = (List<
+ DeprecatedPropertyUtil.PropertyRenamer>) renamersField.get(DeprecatedPropertyUtil.class);
+ ConfigPropertyUpgraderIT.renamers = renamers;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
Review Comment:
I could add a public getter but I also kind of dislike how it's set up in general as the ranamers field should really just be an Immutable list as well. The issue of course is if I make it Immutable then we can't change it for testing and other tests do modify it already like DeprecatedPropertyUtilTest. What I might do is make a second static method that takes a list of renamers and the original can delegate so we can pass in whatever we want for testing.
DeprecatedPropertyUtilTest does test the behavior already but it's still nice to have an integration test also test it and not just a unit 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] cshannon commented on pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#issuecomment-1358473182
Hmm, this was the first time I tried squash/merge from the GitHub app and it didn't give me an option to change the commit message like when using a browser. Lesson learned to just use the desktop I guess or figure out how to get it to prompt for a new message.
--
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 a diff in pull request #3135: Add back in manager PropertyRenamer to ConfigPropertyUpgraderIT to fix test
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3135:
URL: https://github.com/apache/accumulo/pull/3135#discussion_r1051620375
##########
test/src/main/java/org/apache/accumulo/test/upgrade/ConfigPropertyUpgraderIT.java:
##########
@@ -62,13 +65,35 @@ public class ConfigPropertyUpgraderIT {
private static ZooKeeper zooKeeper;
private static ZooReaderWriter zrw;
+ // Create legacy renamer for this test
+ private static final DeprecatedPropertyUtil.PropertyRenamer MASTER_MANAGER_RENAMER =
+ DeprecatedPropertyUtil.PropertyRenamer.renamePrefix("master.",
+ Property.MANAGER_PREFIX.getKey());
+ private static List<DeprecatedPropertyUtil.PropertyRenamer> renamers;
+
private InstanceId instanceId = null;
@TempDir
private static File tempDir;
+ static {
+ try {
+ // Make the renamers list visible to be modified
+ final Field renamersField = DeprecatedPropertyUtil.class.getDeclaredField("renamers");
+ renamersField.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ List<DeprecatedPropertyUtil.PropertyRenamer> renamers = (List<
+ DeprecatedPropertyUtil.PropertyRenamer>) renamersField.get(DeprecatedPropertyUtil.class);
+ ConfigPropertyUpgraderIT.renamers = renamers;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
Review Comment:
I could add a public getter but I also kind of dislike how it's set up in general as the ranamers field should really just be an Immutable list as well. The issue of course is if I make it Immutable then I can't change it for testing. What I might do is make a second static method that takes a list of renamers and the original can delegate so we can pass in whatever we want for testing.
--
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