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 14:43:15 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3106: Remove deprecated properties
cshannon opened a new pull request, #3106:
URL: https://github.com/apache/accumulo/pull/3106
Clean up and remove several deprecated properties for the 3.0.0 release
This closes #3105
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1038870819
##########
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:
I wonder if it is better to try and create some fake properties for the tests or something to check the resolve method vs leaving in stubs for non existent properties. Seems like if we want to check the resolve method we should create a test class specifically for that.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1039001504
##########
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:
So I came up with the idea to take two properties (I just picked two random ones) and just mark them as deprecated just for the test using reflection and then restore them at the end. This way we didn't have to do any other magic and the test will always work going forward. It seemed like the simplest/cleanest solution in this case. It's a little hacky to have to use reflection to do this but I think it's ok in this case since it's just a unit test. Let me know what you think. I also fixed/updated my PR based on your other comments.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#issuecomment-1352310446
@ctubbsii - I forgot this PR was still open as well but I will have to fix the merge conflicts, probably Friday.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1039700912
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1685,9 +1685,7 @@ public void removeInUseLogs(Set<DfsLogger> candidates) {
public void checkIfMinorCompactionNeededForLogs(List<DfsLogger> closedLogs) {
// grab this outside of tablet lock.
- @SuppressWarnings("deprecation")
- Property prop = tableConfiguration.resolve(Property.TSERV_WAL_MAX_REFERENCED,
- Property.TSERV_WALOG_MAX_REFERENCED, Property.TABLE_MINC_LOGS_MAX);
+ Property prop = tableConfiguration.resolve(Property.TSERV_WAL_MAX_REFERENCED);
int maxLogs = tableConfiguration.getCount(prop);
Review Comment:
This is another where there's only one property to resolve now. Can just inline the prop variable and get rid of the resolve call.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer1/BulkImport.java:
##########
@@ -207,10 +207,8 @@ public static String prepareBulkImport(ServerContext manager, final VolumeManage
final UniqueNameAllocator namer = manager.getUniqueNameAllocator();
AccumuloConfiguration serverConfig = manager.getConfiguration();
- @SuppressWarnings("deprecation")
ExecutorService workers = ThreadPools.getServerThreadPools().createExecutorService(serverConfig,
- serverConfig.resolve(Property.MANAGER_RENAME_THREADS, Property.MANAGER_BULK_RENAME_THREADS),
- false);
+ serverConfig.resolve(Property.MANAGER_RENAME_THREADS), false);
Review Comment:
```suggestion
Property.MANAGER_RENAME_THREADS, false);
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -146,15 +146,6 @@ public void execute(String[] args) throws Exception {
}
}
- // Remove the tracers, we don't use them anymore.
- @SuppressWarnings("deprecation")
- String path = siteConf.get(Property.TRACE_ZK_PATH);
- try {
- zapDirectory(zoo, path, opts);
Review Comment:
> Do we need to remove this as a final step in the upgrade from 2.1 -> 3.0? I didn't look to see if this is already done.
This should already be done in 2.1. We don't need to keep this for 3.0 upgrade, but we could suggest users remove the `/tracer` directory themselves, or run the ZooZap util with the property still set (which may be done already on `bin/accumulo-cluster stop` already.
##########
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:
clever! I like 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 a diff in pull request #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1044492218
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -146,15 +146,6 @@ public void execute(String[] args) throws Exception {
}
}
- // Remove the tracers, we don't use them anymore.
- @SuppressWarnings("deprecation")
- String path = siteConf.get(Property.TRACE_ZK_PATH);
- try {
- zapDirectory(zoo, path, opts);
Review Comment:
@ctubbsii - Is there a good place to log a warning for this? I don't see an Upgrader class yet for 3.0 so is that something we should create in another PR?
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#issuecomment-1354594071
Ok I fixed the merge conflicts so it should be ready to go.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1039006172
##########
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:
I should also add I considered trying to use mocking and use EasyMock to mock the enum values but enums are final so you can't use normal mocking with EasyMock. I haven't used it before but I think the only way it could work was something like PowerMock but I checked and it is only set up for Junit 4 and not Junit 5. So it seemed like this was a simpler solution than trying to figure out a way to use PowerMock/EasyMock as getting that set up seems out of scope of this PR and if we wanted to do that should be another Issue/PR.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#issuecomment-1336174325
I'd like a couple people to take a look to make sure I didn't screw anything up when I was removing deprecated properties. I ran through the sunny tests and they passed but we should also kick off a full IT if it looks good. As I noted in #3105, this is the first pass to remove most of the properties that were easy to clean up. Follow on issues will be done for more in depth changes for things like compaction strategy.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1038935572
##########
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:
Ideally, we would want to create test properties that aren't mixed in with the other properties... so we don't have to filter them out everywhere we enumerate real properties. If the method can be made generic in some way, perhaps something like `public <T> T resolve(Enumeration<T> .... propEnums);`, then it might be easy to create a test enum class for testing the method. However, the method does depend on being able to read the properties from the AccumuloConfiguration to see if they are set there, so it's not immediately obvious to me how we'd make that work without making it super convoluted.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1051051847
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -146,15 +146,6 @@ public void execute(String[] args) throws Exception {
}
}
- // Remove the tracers, we don't use them anymore.
- @SuppressWarnings("deprecation")
- String path = siteConf.get(Property.TRACE_ZK_PATH);
- try {
- zapDirectory(zoo, path, opts);
Review Comment:
I don't think we need a warning for this. I meant suggest it in the release notes. Accumulo no longer has to care about the existence of this directory. It should have attempted removal on upgrading to 2.1, and if it missed it, it's not a big deal.
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [accumulo] cshannon commented on pull request #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#issuecomment-1344614407
I updated to remove the deprecated HadoopLogCloser class as well as it was moved to a new location and the deprecated property is removed as part of this PR
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #3106:
URL: https://github.com/apache/accumulo/pull/3106
--
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 #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#issuecomment-1344216458
Ok I updated again with the latest feedback, I think I finally got rid of all the places using resolve where it's no longer necessary.
--
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] dlmarion commented on a diff in pull request #3106: Remove deprecated properties
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3106:
URL: https://github.com/apache/accumulo/pull/3106#discussion_r1039542689
##########
server/base/src/test/java/org/apache/accumulo/server/master/balancer/TableLoadBalancerTest.java:
##########
@@ -146,10 +146,6 @@ public void test() {
TableConfiguration conf = createMock(TableConfiguration.class);
// Eclipse might show @SuppressWarnings("removal") as unnecessary.
Review Comment:
Can we get rid of these comments also?
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -146,15 +146,6 @@ public void execute(String[] args) throws Exception {
}
}
- // Remove the tracers, we don't use them anymore.
- @SuppressWarnings("deprecation")
- String path = siteConf.get(Property.TRACE_ZK_PATH);
- try {
- zapDirectory(zoo, path, opts);
Review Comment:
Do we need to remove this as a final step in the upgrade from 2.1 -> 3.0? I didn't look to see if this is already done.
--
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