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/05/17 04:03:13 UTC
[GitHub] [accumulo] kitswas opened a new pull request, #2712: Rename property #2618
kitswas opened a new pull request, #2712:
URL: https://github.com/apache/accumulo/pull/2712
- Added property with new name
- Marked old property as Deprecated
- Updated usages
Fixes #2618
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136624641
> @ctubbsii Could you replace `var` with `Property`? Or is `var` in common use in this repository?
@kitswas `var` is commonly used in this project. In this case, the type is obvious, so I think var is okay, but I can change it, since it doesn't really help that much.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874805835
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
> May want to look at what other properties are doing. I thought we had a replacement mechanism that other code had used.
```
serverConfig.resolve(Property.MANAGER_RENAME_THREADS, Property.MANAGER_BULK_RENAME_THREADS)
```
The property `MANAGER_BULK_RENAME_THREADS` seems to be undergoing deprecation in similar versions.
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1129494731
> I'm not sure what the problem is. It won't be called on the base class... it will call isPropertySet() on one of the implementing classes that implement it. You don't need to catch either NullPointerException or UnsupportedOperationException.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874789076
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -244,9 +244,12 @@ public enum Property {
GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false", PropertyType.BOOLEAN,
"Enables tracing functionality using OpenTelemetry (assuming OpenTelemetry is configured).",
"2.1.0"),
+ @Deprecated
Review Comment:
Fixed in commit b086f0234deac1b629713aa488a5fad2944de665
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136671487
> Thanks for working on this. It was interesting how those partially mocked objects in the tests messed with the ability to fix this easily, but I'm glad for the opportunity to fix those tests.
In retrospect, commit 6189662b0766debf42d199e148e8f52a9c6fe13c and the tests came close to the scenario described in the [Broken windows theory](https://blog.codinghorror.com/the-broken-window-theory/). If commit 6189662b0766debf42d199e148e8f52a9c6fe13c were kept, it would have been an excuse to skip fixing the tests in future.
> If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. 😺
Very tempting, almost dangerously so, but I think I will pass. My passion (programming, computers) is different from my profession. It has always been so. Hopefully, they will merge after I graduate.
I primarily use Windows. [Apache Accumulo does not like that](https://stackoverflow.com/a/15540833/8659747). While I can [build using WSL](https://stackoverflow.com/a/72335829/8659747), WSL does not mesh well with most GUI tools (IntelliJ, Gitkraken, ...) other than VS Code.
> Thanks for the PR @kitswas. If you wish to be added as a contributor to https://accumulo.apache.org/people/, please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/main/pages/people.md and leave a reference to apache/accumulo#2712 in your commit log.
Is it alright to do this when you did most of the work?
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1132111435
> > Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
>
> Any related issue(s)?
I don't understand this question.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136704746
> In retrospect, commit [6189662](https://github.com/apache/accumulo/commit/6189662b0766debf42d199e148e8f52a9c6fe13c) and the tests came close to the scenario described in the [Broken windows theory](https://blog.codinghorror.com/the-broken-window-theory/). If commit [6189662](https://github.com/apache/accumulo/commit/6189662b0766debf42d199e148e8f52a9c6fe13c) were kept, it would have been an excuse to skip fixing the tests in future.
Thanks for the good read. I was roughly familiar with the concept, but I hadn't thought about it in terms of programming. Seems obvious now. :smiley_cat:
>
> > If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. smiley_cat
>
> Very tempting, almost dangerously so, but I think I will pass. My passion (programming, computers) is different from my profession. It has always been so. Hopefully, they will merge after I graduate.
Many Apache projects have contributors who contribute as a hobby. You're not alone there :smiley_cat:
>
> I primarily use Windows. [Apache Accumulo does not like that](https://stackoverflow.com/a/15540833/8659747). While I can [build using WSL](https://stackoverflow.com/a/72335829/8659747), WSL does not mesh well with most GUI tools (IntelliJ, Gitkraken, ...) other than VS Code.
While it is highly unlikely that Accumulo will ever support Windows as a production platform, it's pretty common to do development on Windows. If there are small things here and there that we can do to make that easier, I think we'd be open to pull requests. But if there isn't an active interest to maintain that support, it will likely break over time. It's possible that future improvements to WSL will make Windows more seamless with Windows. You could also try running a virtual machine. Although, my personal recommendation is to just get used to using Linux :smiley_cat:
>
> > Thanks for the PR @kitswas. If you wish to be added as a contributor to https://accumulo.apache.org/people/, please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/main/pages/people.md and leave a reference to #2712 in your commit log.
>
> Is it alright to do this when you did most of the work?
I would disagree with that assessment. I did the work on the tests, but those were merged separately. You did the work for this PR, and you helped identify the bugs in the tests that inspired those changes. My alterations to this PR ended up being very minimal code. But that's normal collaboration / code review. You were a part of that and can still take credit for your part in the collaboration!
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1129074344
The method `AccumuloConfiguration.resolve()`
https://github.com/apache/accumulo/blob/e52df66b944e3e15098fa3ff65ff9f05aad6afc2/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L99
calls the method `isPropertySet()`
https://github.com/apache/accumulo/blob/e52df66b944e3e15098fa3ff65ff9f05aad6afc2/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L410
which has _7 overrides_.
Using the `.resolve()` of our argument `conf` throws an `UnsupportedOperationException`.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874793617
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
May want to look at what other properties are doing. I thought we had a replacement mechanism that other code had used.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874818857
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
@ctubbsii
https://github.com/apache/accumulo/pull/2712#discussion_r874805835
Should I use this?
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136606490
> @kitswas I accidentally reverted your last "Code cleanup" commit, but I see what you were trying to do. I want to keep the warning narrow on only the old property, so I added another commit that I think gets what you were trying to do, but preserves my intention as well. It's the same number of lines.
@ctubbsii Could you replace `var` with `Property`? Or is `var` in common use in this repository?
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1132835553
> Any proposed changes here should include any necessary adjustments to tests that would otherwise be left broken if the changes were accepted.
Guidance requested.
> Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
"That should never happen in runtime. " -
Then why was it in the tests? Should the tests be rewritten or,
considering the tests were for an _impossible_ condition, be deleted outright?
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1132658080
> > > Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
> >
> >
> > Any related issue(s)?
>
> I don't understand this question.
I wanted to ask if there are existing [issues](https://github.com/apache/accumulo/issues) in this repository related to this topic (`the tests need to be fixed`).
@ctubbsii
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874893543
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
Implemented in 7f1df30ebc8754bfac3a361db4db1530a54194f4.
--
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] milleruntime commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1128778421
Thanks @kitswas. Whenever you are ready, please click the "Ready for review" button to convert your PR from draft status.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874800915
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
> I'm not sure this is sufficient. The old property is deprecated, not removed. This means that users may have the old property, and not the new one, in their configuration. I think you need to check for both and maybe use the one that has a larger value.
Since `createGeneralScheduledExecutorService()` creates a new service, I felt it should use the new property `GENERAL_THREADPOOL_SIZE` to do so.
You suggestion, to use the one that has a larger value, is valid.
Alternatively, for backward compatibility, we can have an overloaded version of the function that uses the new property and leave the old function as it is.
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1134955212
```
[ERROR] Errors:
[ERROR] CompactionCoordinatorTest.testCoordinatorColdStart:274 » NullPointer
[ERROR] CompactionCoordinatorTest.testCoordinatorColdStartNoCompactions:217 » NullPointer
[ERROR] CompactionCoordinatorTest.testCoordinatorRestartNoRunningCompactions:347 » NullPointer
[ERROR] CompactionCoordinatorTest.testCoordinatorRestartOneRunningCompaction:427 » NullPointer
[ERROR] CompactionCoordinatorTest.testGetCompactionJob:510 » NullPointer
[ERROR] CompactionCoordinatorTest.testGetCompactionJobNoJobs:581 » NullPointer
```
The culprit is the line
```java
AccumuloConfiguration conf = PowerMock.createNiceMock(AccumuloConfiguration.class);
```
6 copies of this line exist in the file [`CompactionCoordinatorTest.java`](https://github.com/apache/accumulo/blob/main/server/compaction-coordinator/src/test/java/org/apache/accumulo/coordinator/CompactionCoordinatorTest.java), each responsible for a `NullPointerError` shown above.
Tried replacing the abstract class `AccumuloConfiguration` with child classes (`DefaultConfiguration` and `SiteConfiguration`) but to no avail.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r875408567
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -512,10 +516,16 @@ public Future<?> submit(Runnable task) {
* If you need the server-side shared ScheduledThreadPoolExecutor, then use
* ServerContext.getScheduledExecutor()
*/
+ @SuppressWarnings("deprecation")
Review Comment:
I will do this when Exception Handling is no longer required.
Otherwise, it only serves to clutter the code.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874856725
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
That looks like what I was thinking.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1129458437
> The method `AccumuloConfiguration.resolve()`
>
> https://github.com/apache/accumulo/blob/e52df66b944e3e15098fa3ff65ff9f05aad6afc2/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L99
>
>
> calls the method `isPropertySet()`
> https://github.com/apache/accumulo/blob/e52df66b944e3e15098fa3ff65ff9f05aad6afc2/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L410
>
>
> which has _7 overrides_.
> 
>
> Using the `.resolve()` of our argument `conf` throws an `UnsupportedOperationException`.
>
> How should I proceed? Should I simply generalize the catch clause to `Exception`?
I'm not sure what the problem is. It won't be called on the base class... it will call isPropertySet() on one of the implementing classes that implement it. You don't need to catch either NullPointerException or UnsupportedOperationException.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2712:
URL: https://github.com/apache/accumulo/pull/2712
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1135321175
> > Tests often use mock objects to test specific code paths. When you change the behavior of the code, you can break certain assumptions that the test code is relying on.
>
> Instead of breaking those assumptions, commit [6189662](https://github.com/apache/accumulo/commit/6189662b0766debf42d199e148e8f52a9c6fe13c) tries to conform to them by using the new property `Property.GENERAL_THREADPOOL_SIZE` whenever the conflict resolution fails. The old code used `Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE` in all situations, no matter what the value of the argument `conf` was and happily passed `conf` to `createExecutorService()`.
I still think that this is the wrong direction. I don't think we should broaden the catch clause like this... the tests have been identified as making faulty assumptions while creating partially mocked objects ("nice" mocks). Rather than conform to these faulty assumptions, it would be better to improve the mocking so that they are not making the faulty assumptions in the first place. I will take a look to see what is involved in fixing the tests.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1132856591
> > Any proposed changes here should include any necessary adjustments to tests that would otherwise be left broken if the changes were accepted.
>
> Guidance requested.
>
You'd have to look at each test to see why it's failing. Each test may require something different. It's likely that it's just a trivial fix that is needed... like adding another mocked method call to the set of expected method calls, or implementing a method in a subclass created for testing.
> > Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
>
> "That should never happen in runtime. " - Then why was it in the tests? Should those particular tests be rewritten or, considering the tests were for an _impossible_ condition, be deleted outright?
Tests often use mock objects to test specific code paths. When you change the behavior of the code, you can break certain assumptions that the test code is relying on. Depending on what changed, it's likely the tests just need minor adjustments.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874800915
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
Since `createGeneralScheduledExecutorService()` creates a new service, I felt it should use the new property `GENERAL_THREADPOOL_SIZE` to do so.
You suggestion, to use the one that has a larger value, is valid.
Alternatively, for backward compatibility, we can have an overloaded version of the function that uses the new property and leave the old function as it is.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874893543
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
Implemented in 4a6bd0eac6bd5de996976caf8870b4b87e7daea0.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1130564011
Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136592531
@kitswas I accidentally reverted your last "Code cleanup" commit, but I see what you were trying to do. I want to keep the warning narrow on only the old property, so I added another commit that I think gets what you were trying to do, but preserves my intention as well. It's the same number of lines.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r881075035
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -512,10 +516,16 @@ public Future<?> submit(Runnable task) {
* If you need the server-side shared ScheduledThreadPoolExecutor, then use
* ServerContext.getScheduledExecutor()
*/
+ @SuppressWarnings("deprecation")
Review Comment:
I did it in 58e8ec5b960dd2f6a4c20b4b0c2212ee67b0e5e6
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874742343
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -244,9 +244,12 @@ public enum Property {
GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false", PropertyType.BOOLEAN,
"Enables tracing functionality using OpenTelemetry (assuming OpenTelemetry is configured).",
"2.1.0"),
+ @Deprecated
Review Comment:
This should also have a `@ReplacedBy` annotation
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
I'm not sure this is sufficient. The old property is deprecated, not removed. This means that users may have the old property, and not the new one, in their configuration. I think you need to check for both and maybe use the one that has a larger value.
--
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] kitswas commented on a diff in pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r874893543
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -515,7 +519,7 @@ public Future<?> submit(Runnable task) {
public ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
return (ScheduledThreadPoolExecutor) createExecutorService(conf,
- Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE, true);
+ Property.GENERAL_THREADPOOL_SIZE, true);
Review Comment:
Implemented in 7f1df30ebc8754bfac3a361db4db1530a54194f4.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#discussion_r875382704
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -512,10 +516,16 @@ public Future<?> submit(Runnable task) {
* If you need the server-side shared ScheduledThreadPoolExecutor, then use
* ServerContext.getScheduledExecutor()
*/
+ @SuppressWarnings("deprecation")
Review Comment:
You can narrow this warnings suppression to a variable rather than the whole method by doing something like:
```java
Property prop = conf.resolve(Property.GENERAL_THREADPOOL_SIZE, Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE);
```
and then put the warnings suppression only on that variable, then use that variable in the next call to createExecutorService.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1132791265
> > > > Okay, it looks like those indicate the problem is likely with using an AccumuloConfiguration object for testing that doesn't override the method. That should never happen in runtime. It seems the tests need to be fixed.
> > >
> > >
> > > Any related issue(s)?
> >
> >
> > I don't understand this question.
>
> I wanted to ask if there are existing [issues](https://github.com/apache/accumulo/issues) in this repository related to this topic (`the tests need to be fixed`). @ctubbsii
Probably not. I wouldn't expect there to be, since the tests work fine as is... they would only broken by the proposed changes here. Any proposed changes here should include any necessary adjustments to tests that would otherwise be left broken if the changes were 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
[GitHub] [accumulo] ctubbsii commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1135563634
@kitswas I fixed the faulty test assumptions in #2727. Once that is merged, we can proceed with these changes, but you won't need to catch or handle those exceptions anymore.
--
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] kitswas commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
kitswas commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1134962207
> Tests often use mock objects to test specific code paths. When you change the behavior of the code, you can break certain assumptions that the test code is relying on.
Instead of breaking those assumptions, commit 6189662b0766debf42d199e148e8f52a9c6fe13c tries to conform to them by using the new property `Property.GENERAL_THREADPOOL_SIZE` whenever the conflict resolution fails.
The old code used `Property.GENERAL_SIMPLETIMER_THREADPOOL_SIZE` in all situations, no matter what the value of the argument `conf` was.
--
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 #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1136630928
I think this is good to merge now. I pinged @dlmarion for an updated review, but I think his comments have already been fully addressed. He can always follow up with an additional review after the fact.
Thanks for working on this. It was interesting how those partially mocked objects in the tests messed with the ability to fix this easily, but I'm glad for the opportunity to fix those tests.
Thanks for the PR @kitswas . If you wish to be added as a contributor to https://accumulo.apache.org/people/ , please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/main/pages/people.md and leave a reference to `apache/accumulo#2712` in your commit log.
If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. :smiley_cat:
--
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] milleruntime commented on pull request #2712: Rename property GENERAL_SIMPLETIMER_THREADPOOL_SIZE
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2712:
URL: https://github.com/apache/accumulo/pull/2712#issuecomment-1137095258
Thanks for the contribution @kitswas. There was a brief time where I only had access to a windows machine and made some small attempts to getting Accumulo to build on Windows. Check out my branch if you are interested: https://github.com/milleruntime/accumulo/tree/windows
--
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