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/03 20:47:41 UTC
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3106: Remove deprecated properties
ctubbsii commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1038868428
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -93,9 +93,8 @@ public static <U> Class<? extends U> loadClass(String className, Class<U> extens
/**
* Retrieve the classloader context from a table's configuration.
*/
- @SuppressWarnings("removal")
public static String tableContext(AccumuloConfiguration conf) {
- return conf.get(conf.resolve(Property.TABLE_CLASSLOADER_CONTEXT, Property.TABLE_CLASSPATH));
+ return conf.get(conf.resolve(Property.TABLE_CLASSLOADER_CONTEXT));
Review Comment:
You should simplify these calls to `resolve`, if there's only one property. The `resolve` method tries to select the property to use based on what the user has set in the config. However, if there's only one, then there's nothing to `resolve` and you can just call `get` on the property directly.
```suggestion
return conf.get(Property.TABLE_CLASSLOADER_CONTEXT);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java:
##########
@@ -198,11 +198,9 @@ public boolean recoverLogs(KeyExtent extent, Collection<Collection<String>> walo
synchronized (this) {
if (!closeTasksQueued.contains(sortId) && !sortsQueued.contains(sortId)) {
AccumuloConfiguration aconf = manager.getConfiguration();
- @SuppressWarnings("deprecation")
LogCloser closer = Property.createInstanceFromPropertyName(aconf,
- aconf.resolve(Property.MANAGER_WAL_CLOSER_IMPLEMENTATION,
- Property.MANAGER_WALOG_CLOSER_IMPLEMETATION),
- LogCloser.class, new HadoopLogCloser());
+ aconf.resolve(Property.MANAGER_WAL_CLOSER_IMPLEMENTATION), LogCloser.class,
Review Comment:
another resolve on single property can be simplified; there's a bunch more, but I'll stop mentioning it.
##########
shell/src/main/java/org/apache/accumulo/shell/Shell.java:
##########
@@ -472,27 +472,13 @@ public ClassLoader getClassLoader(final CommandLine cl, final Shell shellState)
}
private static String getTableContextFromProps(Map<String,String> props) {
- String tableContext = null;
for (Entry<String,String> entry : props.entrySet()) {
- // look for either the old property or the new one, but
- // if the new one is set, stop looking and let it take precedence
if (entry.getKey().equals(Property.TABLE_CLASSLOADER_CONTEXT.getKey())
&& entry.getValue() != null && !entry.getValue().isEmpty()) {
return entry.getValue();
Review Comment:
Since we're only looking for a single property now, I don't think this needs to be in a loop. We should be able to just directly call `props.get(Property.TABLE_CLASSLOADER_CONTEXT.getKey())` and return it.
##########
server/base/src/test/java/org/apache/accumulo/server/master/balancer/TableLoadBalancerTest.java:
##########
@@ -146,9 +146,7 @@ public void test() {
TableConfiguration conf = createMock(TableConfiguration.class);
// Eclipse might show @SuppressWarnings("removal") as unnecessary.
// Eclipse is wrong. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=565271
- @SuppressWarnings("removal")
- Property TABLE_CLASSPATH = Property.TABLE_CLASSPATH;
- expect(conf.resolve(Property.TABLE_CLASSLOADER_CONTEXT, TABLE_CLASSPATH))
+ expect(conf.resolve(Property.TABLE_CLASSLOADER_CONTEXT))
Review Comment:
This would be a weird thing to expect. Probably can be removed when the code that we're expecting is updated to avoid the resolution of a single non-deprecated property, and just uses that property directly.
##########
core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java:
##########
@@ -394,42 +386,17 @@ public void testScanExecutors() {
// note: this is hard to test if there aren't any deprecated properties
// if that's the case, just comment this test out or create a dummy deprecated property
- @SuppressWarnings("deprecation")
@Test
public void testResolveDeprecated() {
var conf = new ConfigurationCopy();
- // deprecated first argument
- var e1 = assertThrows(IllegalArgumentException.class, () -> conf
- .resolve(Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI, Property.INSTANCE_DFS_URI));
- assertEquals("Unexpected deprecated INSTANCE_DFS_DIR", e1.getMessage());
-
// non-deprecated second argument
- var e2 = assertThrows(IllegalArgumentException.class,
- () -> conf.resolve(Property.INSTANCE_VOLUMES, Property.INSTANCE_DFS_DIR,
- Property.INSTANCE_SECRET, Property.INSTANCE_DFS_DIR, Property.INSTANCE_VOLUMES));
+ var e2 = assertThrows(IllegalArgumentException.class, () -> conf
+ .resolve(Property.INSTANCE_VOLUMES, Property.INSTANCE_SECRET, Property.INSTANCE_VOLUMES));
assertEquals("Unexpected non-deprecated [INSTANCE_SECRET, INSTANCE_VOLUMES]", e2.getMessage());
// empty second argument always resolves to non-deprecated first argument
assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES));
- // none are set, resolve to non-deprecated
- assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES,
- Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
-
- // resolve to first deprecated argument that's set; here, it's the final one
- conf.set(Property.INSTANCE_DFS_URI, "");
- assertSame(Property.INSTANCE_DFS_URI, conf.resolve(Property.INSTANCE_VOLUMES,
- Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
-
- // resolve to first deprecated argument that's set; now, it's the first one because both are set
- conf.set(Property.INSTANCE_DFS_DIR, "");
- assertSame(Property.INSTANCE_DFS_DIR, conf.resolve(Property.INSTANCE_VOLUMES,
- Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
-
- // every property is set, so resolve to the non-deprecated one
- conf.set(Property.INSTANCE_VOLUMES, "");
- assertSame(Property.INSTANCE_VOLUMES, conf.resolve(Property.INSTANCE_VOLUMES,
- Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI));
Review Comment:
More tests that can't be tested without a deprecated property, but check the correct behavior of the `resolve` method.
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -470,9 +467,7 @@ public Future<?> submit(Runnable task) {
*/
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
- @SuppressWarnings("deprecation")
- Property oldProp = Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE;
- Property prop = conf.resolve(Property.GENERAL_THREADPOOL_SIZE, oldProp);
+ Property prop = conf.resolve(Property.GENERAL_THREADPOOL_SIZE);
Review Comment:
```suggestion
Property prop = Property.GENERAL_THREADPOOL_SIZE;
```
##########
core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java:
##########
@@ -394,42 +386,17 @@ public void testScanExecutors() {
// note: this is hard to test if there aren't any deprecated properties
// if that's the case, just comment this test out or create a dummy deprecated property
- @SuppressWarnings("deprecation")
@Test
public void testResolveDeprecated() {
var conf = new ConfigurationCopy();
- // deprecated first argument
- var e1 = assertThrows(IllegalArgumentException.class, () -> conf
- .resolve(Property.INSTANCE_DFS_DIR, Property.INSTANCE_DFS_URI, Property.INSTANCE_DFS_URI));
- assertEquals("Unexpected deprecated INSTANCE_DFS_DIR", e1.getMessage());
-
Review Comment:
Some of these tests are testing behavior of the `resolve` method. We may want to leave some kind of stub in place of these. Without a deprecated property to test resolution with, we can't perform these tests... but we will probably want to perform them if/when we deprecate properties in the future.
--
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