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 2020/07/30 07:31:30 UTC
[GitHub] [accumulo] ctubbsii opened a new issue #1665: Preserve user intent with regard to overridden compaction thread pool size
ctubbsii opened a new issue #1665:
URL: https://github.com/apache/accumulo/issues/1665
As mentioned on #1662, the new property `tserver.compaction.major.service.default.planner.opts.executors` overrides any user-specified value in the now deprecated `tserver.compaction.major.concurrent.max` property.
My proposed solution is:
* Change the default value of the old property to: `0`
* If the old property is not configured to its default value, then it must be overridden by the user, in which case, we should:
* Infer the user wishes a single thread pool like: `[{"name":"deprecation-inferred-single-pool","numThreads":"X"}]`, where `X` is the value configured in the old property, and
* Issue a warning that the old property is deprecated and to show the inferred value for the new property.
* If both properties' default values are overridden, then we should:
* Use the new property's value, and
* Issue a warning about the old one being set and show the new property's value that will be used.
* If only the new property, or neither, or overridden from their default values, then the new value will be used with no warnings.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-673204410
> The configuration design pattern is not something new, it follows a precedent set by many other Accumulo configuration properties
Okay, I think I see the pattern you were going for now. We actually have two kinds of "PREFIX" properties: categorical (like `TABLE_PREFIX`, `INSTANCE_PREFIX`, etc.) for logical grouping, and prefacing (like those you mentioned, as well as others that look like `*.opts.*` or `*.custom.*`) which preface user-defined suffixes. Because these new properties aren't exactly categorical, and aren't exactly prefacing, I had difficulty recognizing what you were going for. However, I think I get it now. Thanks for the clarification.
As for offering an alternative... it is difficult to think of alternatives when one doesn't grasp what is there now. The compaction stuff got added too quickly for me to keep up in the past few months. (That's not to say they got added too quickly, objectively speaking... just that due to circumstances, I hadn't been able to follow it as closely over these last few months as I would have needed to in order to understand it fully.) So, I've been stuck at the 'try-to-understand-what-is-there-now' phase. I believe I now have a better idea of what your intent was, and can try to think of alternatives.
I actually have some alternative ideas in mind now, but they aren't concrete yet. If I see you in Slack, maybe I can bounce them off you, before they coalesce into a future pull request. A preview of what's swirling in my head right now: more JSON, fewer separate config properties. Even if I don't see you there, I think I have enough understanding now to confidently move forward on this ticket, anyway.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-754054768
@ctubbsii I am interested in completing this issue, are you still thinking of working on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-670569983
@keith-turner It has turned out to be extremely difficult to track down the appropriate place to implement this. If you get a moment to go over the compaction configuration implementation, I'd love to chat with you about this on Slack.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-672396552
@keith-turner Right, I got that far already. My concern wasn't exactly a question of where to implement it, because I did already track down that code after a significant amount of time. Rather, my concern is in the difficulty in tracking it down itself, as a result of the current design, as well as potential problems with the overall design of this new configuration parsing stuff for compactions.
The core of my concern is this: While other uses of configuration properties are fairly easy to track down, because either the enum or their configuration key is directly referenced in other code, that is not the case for these new compaction properties. These configuration properties are utilized in the code only after being carefully parsed by code that is limited in comments and has no direct references to any of the configuration enums or keys themselves (which would make it much easier to search for references and to ensure changes are correctly made in all places the property is used, if a change is needed; aka maintainability). Instead, these enums seem to exist solely for the documentation generator, and are largely disconnected from the implementation. This somewhat undermines the point of the enum container for configuration properties, as the enums exist to help avoid bugs that can be introduced due to implementation code that diverges from the documentation. I also thi
nk that the complexity, while I admire the JSON-based solution you designed, is partially to blame for the unexpected behavior at the core of this ticket. And, I suspect this ticket won't be the last of its kind pertaining to these properties if we do not adequately address it sooner.
So, I realize that's a fairly critical view of your code... probably coming across more critical than I intend (sorry, if that's the case). But that's why I was hoping to have a live chat... to see if we can come up with something that alleviates my concerns without undermining the intent of your design. I can, of course, bake in a fix for the narrow issue identified in this ticket, but I was hoping to discuss the overall approach. With a better overall approach, I'd have more confidence in a fix for this issue. As it stands, without being certain of everywhere in the code where these properties are used (and it being hard to find them), I'd have a low degree of confidence in the overall correctness of any fix (for this issue, and any others with regard to these configuration properties). Testing can help, but being able to follow the code with a better overall more readable/maintainable design, would also help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-673427664
> > So, I've been stuck at the 'try-to-understand-what-is-there-now' phase.
>
> Have you looked at the Javadocs and this [apache/accumulo-website#232](https://github.com/apache/accumulo-website/pull/232) ?
Yes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-754074194
> @ctubbsii I am interested in completing this issue, are you still thinking of working on it?
@keith-turner I'll leave it to you, then. I'm busy with other tasks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-673245207
> So, I've been stuck at the 'try-to-understand-what-is-there-now' phase.
Have you looked at the Javadocs and this https://github.com/apache/accumulo-website/pull/232 ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-673155182
> Rather, my concern is in the difficulty in tracking it down itself, as a result of the current design, as well as potential problems with the overall design of this new configuration parsing stuff for compactions.
@ctubbsii the code can easily be found by searching for references to `Property.TSERV_COMPACTION_SERVICE_PREFIX`. The configuration design pattern is not something new, it follows a precedent set by many other Accumulo configuration properties that need to allow a user to configure one or more of something. For other examples see `Property.TABLE_CONSTRAINT_PREFIX`, `Property.TABLE_ITERATOR_PREFIX`, etc.
> Instead, these enums seem to exist solely for the documentation generator, and are largely disconnected from the implementation.
The defaults under `Property.TSERV_COMPACTION_SERVICE_PREFIX` will be used to create the default compaction services and are not just for documentation.
> So, I realize that's a fairly critical view of your code... probably coming across more critical than I intend (sorry, if that's the case).
I welcome criticism. You have made this criticism multiple times before. I think it would be more constructive to offer an alternative solution rather than repeatedly criticize this w/o ever offering an alternative. I am not going to spend time thinking about alternatives because I think what is there is fine. If someone else came up with an alternative and I thought it was better, I would be the first to say hey lets ditch the old stuff. I would be happy to meet and discuss this further I will try to connect with you on Slack.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner closed issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
keith-turner closed issue #1665:
URL: https://github.com/apache/accumulo/issues/1665
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1665: Preserve user intent with regard to overridden compaction thread pool size
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1665:
URL: https://github.com/apache/accumulo/issues/1665#issuecomment-672318852
@ctubbsii I think I would work it into the following code.
https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java#L65-L88
Make this code check if the old setting are set and the new settings are defaults, then it could insert special values into the `planners` and `options` maps that would cause a service to be created with a single thread pool of the specified size.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org