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/06/23 16:21:58 UTC

[GitHub] [accumulo] keith-turner opened a new issue #1640: Rename Accumulo master properties

keith-turner opened a new issue #1640:
URL: https://github.com/apache/accumulo/issues/1640


   After a new name is selected (#1638 and #1639) we can start making code changes.  One major change will be renaming the Accumulo master properties.  This is a user facing element so we need to come up with a good strategy for doing, which can be done before the name is selected.  The strategy can be discussed here.


----------------------------------------------------------------
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 edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-761014093


   > Extra complexity
   
   I agree that lots of complexity should not be added to the code to support this one case. The complexity of the Accumulo code base could be considered in conjunction w/ the complexity of the user experience.  Having sets of deprecated properties w/ different runtime behaviors is a complicated user experience.  An ideal solution would reduce the complexity of both, not sure it exists though.
   
   > Properties are rarely simply renamed..
   
   That is a good point.  There was not a 1:1 relationship between the deprecated prop and replacements in the recent PR #1861 I did.
   
   > I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued
   
   This is really its own issue, but this comment made me think of the following. If we wanted to adopt something more aggressive than semver for removing deprecated props, doing it before 2.1 is released is a good time. Thinking any prop deprecated before an LTS release could be removed in main after the LTS is released (this would be more relaxed that semver possibly).  Not sure about non-LTS releases.  The new policy could be documented and warnings logged for deprecated props could mention when the removal will happen.
   


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760438448


   > With my suggestion, they would still go through a deprecation cycle of a sort. 
   
   Having sets of deprecated properties that behave in very different ways would be confusing from a user perspective.  It would need to be reflected in the documentation, so users know what to do. 
   
   > We could put in the additional effort of allowing such properties by translating any incoming property changes intended for the ZooKeeper store via the API from master.* to manager.* to support the automation you describe. 
   
   Could try to put specialized handling in AccumuloConfiguration to map these props on lookup.  Like what resolve does, but not spreading it everywhere in the code.  This may not be additional effort but not sure.  
   
   Thinking it would be nice to centralize the handling of this large group of deprecated properties in the code (avoiding specialized per property handling in the code), but make them behave like every other deprecated property in Accumulo from a users perspective. 


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-761014093


   > Extra complexity
   
   I agree that lots of complexity should not be added to the code to support this one case. The complexity of the Accumulo code base could be considered in conjunction w/ the complexity of the user experience.  Having sets of deprecated properties w/ different runtime behaviors is a complicated user experience.  An ideal solution would reduce the complexity of both, not sure it exists though.
   
   > Properties are rarely simply renamed..
   
   There is a good point.  There was not a 1:1 relationship between the deprecated prop and replacements in the recent PR #1861 I did.
   
   > I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued
   
   This is really its own issue, but this comment made me think of the following. If we wanted to adopt something more aggressive than semver for removing deprecated props, doing it before 2.1 is released is a good time. Thinking any prop deprecated before an LTS release could be removed in main after the LTS is released (this would be more relaxed that semver possibly).  Not sure about non-LTS releases.  The new policy could be documented and warnings logged for deprecated props could mention when the removal will happen.
   


----------------------------------------------------------------
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 edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760621401

@brianloss There are a few concerns I'd have with that proposal:

1. Extra complexity (it seems a little more complicated and a lot more code changing than the previous proposals),
2. I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued (this plan sounds like the intent is to support "old names" in perpetuity; in this case, we actually want to discontinue the use of the term "master" in its entirety; this is possibly true for other cases, as well), and
3. Properties are *rarely* simply renamed... they are most often changed because of a shift in their semantics. Supporting "old names" implies simple renames. That is the case for this change, but this is, as I've said, a special case.

I agree with @keith-turner 's concern about having a gigantic list of individually deprecated properties in the docs. I had been thinking about that as well. In the proposal I suggested, it would be simple to add a section to the generated properties documentation to document that (and how) the `master.` prefix is deprecated.

----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-761153214


   I don't think we should adopt anything more aggressive than semver for deprecated props, but I do think this is a special case that may need to be considered apart from what we might do "normally", and that it's okay to flex semver under special circumstances. However, I don't think we need to do that here. Given the conversation indicating a desire to keep support for setting new properties in the API using the old names, I think we can preserve deprecation consistency with very simple changes, that keep complexity low for users and maintainers:
   
   1. remap SiteConfiguration props on load,
   2. remap ZooConfiguration on upgrade, and
   3. remap props for setProperty() on API call.
   
   (All with appropriate log messages and updated documentation, of course.)
   
   The remapping function could even be located in a single place, to make it easier to drop support for the deprecated props in the next major release.


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760419434

> That means these props would not be going through a deprecation cycle? That could have a negative impact on any automation developed around Accumulo. Its not always user sitting at terminals setting properties.

With my suggestion, they would still go through a deprecation cycle. While deprecated, we would still respect existing config files, but yes, it would disallow automation of the type you describe, altering these properties in ZooKeeper... at least, without any additional effort.

We could put in the additional effort of allowing such properties by translating any incoming property changes intended for the ZooKeeper store via the API from `master.*` to `manager.*` to support the automation you describe. However, I'm not sure this would really be worth it... because automation would already be negatively impacted because the returned properties when querying would not show the old properties. So, if there were automation reading, modifying, and writing, it wouldn't be able to see the `master.*` properties.

I'm fine with not adding in this additional effort, though, because I think the risk is pretty low. Automation of the type that would change the `master.*` properties without human intervention seems like it'd be a rare thing, if it exists at all.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] brianloss commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760402227


   @ctubbsii I don't know why I assumed Property was in the public API but since it's not, I agree your approach will be cleaner. Thanks!


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760407386


   > @ctubbsii I like your proposal, it seems to imply that users would no longer be able to set master props in zookeeper.  Was that what you were thinking?
   
   Yes it was. We would still support reading an old config file, but would otherwise prevent any new properties from being set using the old names.


----------------------------------------------------------------
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] brianloss commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760455032


   > > With my suggestion, they would still go through a deprecation cycle of a sort.
   > 
   > Having sets of deprecated properties that behave in very different ways would be confusing from a user perspective. It would need to be reflected in the documentation, so users know what to do.
   
   I was wondering about this too. Even though the class Property is not in the public API, by having a mechanism to deprecate properties and using that mechanism, including on `master.bulk.rename.threadpool.size`, it seems we're treating them as though they are part of the public API.
   
   > Could try to put specialized handling in AccumuloConfiguration to map these props on lookup. Like what resolve does, but not spreading it everywhere in the code. This may not be additional effort but not sure.
   
   Maybe this idea is worth exploring? It seems like possibly it could be done in the `get(Property)` method.


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760621401

@brianloss There are a few concerns I'd have with that proposal:

1. Extra complexity (it seems a little more complicated and a lot more code changing than the previous proposals),
2. I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued (this plan sounds like the intent is to support "old names" in perpetuity; in this case, we actually want to discontinue the use of the term "master" in its entirety; this is possibly true for other cases, as well, however, for many other cases), and
3. Properties are *rarely* simply renamed... they are most often changed because of a shift in their semantics. Supporting "old names" implies simple renames. That is the case for this change, but this is, as I've said, a special case.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] keith-turner edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760465143


   > it seems we're treating them as though they are part of the public API.
   
   I think this treatment derives from the fact that they are user facing.  I am uncertain about the best path in this particular situation, but I do know its good to give user facing changes a bit more consideration.  The conversation is making me think about property deprecation in Accumulo in general.


----------------------------------------------------------------
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] brianloss edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760938382


   > @brianloss There are a few concerns I'd have with that proposal:
   > 
   > 1. Extra complexity (it seems a little more complicated and a lot more code changing than the previous proposals),
   
   I'm not sure it's that complicated since the changes would be pretty much limited to Property and AccumuloConfiguration (changes to the AccumuloConfiguration sub-classes would be through IDE refactoring, and all of the property enum renames and their cascading changes would happen with any approach). I had been thinking I might prototype it and see, but the idea was just a thought, and see below...
   
   > 2. I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued (this plan sounds like the intent is to support "old names" in perpetuity; in this case, we actually want to discontinue the use of the term "master" in its entirety; this is possibly true for other cases, as well), and
   
   I wasn't intending that we'd support the old names indefinitely, but rather follow semver rules (even though we don't _have_ to for the properties) and have them be fully supported until the next major release when they could be removed.
   
   > 3. Properties are _rarely_ simply renamed... they are most often changed because of a shift in their semantics. Supporting "old names" implies simple renames. That is the case for this change, but this is, as I've said, a special case.
   > 
   > I agree with @keith-turner 's concern about having a gigantic list of individually deprecated properties in the docs. I had been thinking about that as well. In the proposal I suggested, it would be simple to add a section to the generated properties documentation to document that (and how) the `master.` prefix is deprecated.
   
   That's a fair point about simple renames. If we don't think they're likely to happen again any time soon **and** we don't think our user community will be upset about a partially incompatible change (not being able to use set the old property names in Zookeeper) on a minor version release, then I can just implement what you suggested. I also agree the documentation is important and we wouldn't want the noise of a huge list of deprecated properties in the docs.


----------------------------------------------------------------
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] brianloss closed issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss closed issue #1640:
URL: https://github.com/apache/accumulo/issues/1640


   


----------------------------------------------------------------
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] brianloss commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760938382


   > @brianloss There are a few concerns I'd have with that proposal:
   > 
   > 1. Extra complexity (it seems a little more complicated and a lot more code changing than the previous proposals),
   
   I'm not sure it's that complicated since the changes would be pretty much limited to Property and AccumuloConfiguration (changes to the AccumuloConfiguration sub-classes would be through IDE refactoring). I had been thinking I might prototype it and see, but the idea was just a thought, and see below...
   
   > 2. I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued (this plan sounds like the intent is to support "old names" in perpetuity; in this case, we actually want to discontinue the use of the term "master" in its entirety; this is possibly true for other cases, as well), and
   
   I wasn't intending that we'd support the old names indefinitely, but rather follow semver rules (even though we don't _have_ to for the properties) and have them be fully supported until the next major release when they could be removed.
   
   > 3. Properties are _rarely_ simply renamed... they are most often changed because of a shift in their semantics. Supporting "old names" implies simple renames. That is the case for this change, but this is, as I've said, a special case.
   > 
   > I agree with @keith-turner 's concern about having a gigantic list of individually deprecated properties in the docs. I had been thinking about that as well. In the proposal I suggested, it would be simple to add a section to the generated properties documentation to document that (and how) the `master.` prefix is deprecated.
   
   That's a fair point about simple renames. If we don't think they're likely to happen again any time soon **and** we don't think our user community will be upset about a partially incompatible change (not being able to use set the old property names in Zookeeper) on a minor version release, then I can just implement what you suggested. I also agree the documentation is important and we wouldn't want the noise of a huge list of deprecated properties in the docs.


----------------------------------------------------------------
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] brianloss commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760380660


   I was thinking of taking this on. In looking at the Properties class, it appears it already supports `Deprecated` and `ReplacedBy` annotations on properties. Given that, I was thinking something like the following could work:
   
   1. Copy all `MASTER_` properties and make a `MANAGER_` equivalent.
   2. Deprecate all `MASTER_` properties and set the `ReplacedBy` property to the corresponding `MANAGER_` property.
   3. Anywhere a `MASTER_` property is used, retrieve either the new or old property using [AccumuloConfiguration.resolve](https://github.com/apache/accumulo/blob/bdcab448b5c4fb44118197615fe2e231816da01c/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L97).
   
   Does that seem like a reasonable 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] keith-turner commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760465143


   > it seems we're treating them as though they are part of the public API.
   
   I think this treatment derives from the fact that they are user facing.  I am uncertain about the best path is in the particular situation, but I do know its good to give user facing changes a bit more consideration.  The conversation is making me think about property deprecation in Accumulo in general.


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760468313


   > Having sets of deprecated properties that behave in very different ways would be confusing from a user perspective. It would need to be reflected in the documentation, so users know what to do.
   > 
   
   This is kind of a special case. I think we could document it well enough, and provide meaningful error/warning messages, that consistency with regular property deprecations won't matter much, because it will be clear what to do from the documentation and/or messages.


----------------------------------------------------------------
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] ivakegg commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ivakegg commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760441639


   I am thinking we may be too nice here.  I propose:
   1) Fail to startup if any of the master.* properties still exist with an informative message.
   2) Use the version upgrade mechanism to fix any overrides in zookeeper.


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760411494


   > Yes it was. We would still support reading an old config file, but would otherwise prevent any new properties from being set using the old names.
   
   That means these props would not be going through a deprecation cycle? That could have a negative impact on any automation developed around Accumulo.  Its not always user sitting at terminals setting properties. 


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-670340928


   The name agreed to in a vote on the mailing list was 'manager'.


----------------------------------------------------------------
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 #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760393519


   @brianloss I think it might be a bit burdensome to use `AccumuloConfiguration.resolve` in so many places. Also, it might get complicated if we are monitoring any properties for changes (I'm not sure if we are). As an alternative, I was thinking we could:
   
   1. Produce an error if both `master.*` and `manager.*` properties exist in the `SiteConfiguration` file on startup and fail to start,
   2. Automatically remap any `master.*` to `manager.*` when loading the `SiteConfiguration` file, issuing a warning if any `master.*` are detected, and
   3. On upgrade, rename any properties in ZooKeeper automatically.
   
   That way, we don't need to maintain any deprecated properties side-by-side in `Property.java` for this, or do anything special to deal with `AccumuloConfiguration.resolve`. We can simply rename the enums and property keys in `Property.java`.


----------------------------------------------------------------
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] brianloss edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760938382


   > @brianloss There are a few concerns I'd have with that proposal:
   > 
   > 1. Extra complexity (it seems a little more complicated and a lot more code changing than the previous proposals),
   
   I'm not sure it's that complicated since the changes would be pretty much limited to Property and AccumuloConfiguration (changes to the AccumuloConfiguration sub-classes would be through IDE refactoring, and all of the property renames and their cascading changes would happen with any approach). I had been thinking I might prototype it and see, but the idea was just a thought, and see below...
   
   > 2. I think we should encourage phase out of these deprecated stuffs, not continued support indefinitely for code or properties that have been discontinued (this plan sounds like the intent is to support "old names" in perpetuity; in this case, we actually want to discontinue the use of the term "master" in its entirety; this is possibly true for other cases, as well), and
   
   I wasn't intending that we'd support the old names indefinitely, but rather follow semver rules (even though we don't _have_ to for the properties) and have them be fully supported until the next major release when they could be removed.
   
   > 3. Properties are _rarely_ simply renamed... they are most often changed because of a shift in their semantics. Supporting "old names" implies simple renames. That is the case for this change, but this is, as I've said, a special case.
   > 
   > I agree with @keith-turner 's concern about having a gigantic list of individually deprecated properties in the docs. I had been thinking about that as well. In the proposal I suggested, it would be simple to add a section to the generated properties documentation to document that (and how) the `master.` prefix is deprecated.
   
   That's a fair point about simple renames. If we don't think they're likely to happen again any time soon **and** we don't think our user community will be upset about a partially incompatible change (not being able to use set the old property names in Zookeeper) on a minor version release, then I can just implement what you suggested. I also agree the documentation is important and we wouldn't want the noise of a huge list of deprecated properties in the docs.


----------------------------------------------------------------
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 edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ctubbsii edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760419434

With my suggestion, they would still go through a deprecation cycle of a sort. While deprecated, we would still respect existing config files (property reads), but yes, it would disallow automation of the type you describe (property writes) that alters these properties in ZooKeeper... at least, without any additional effort. It would also disallow mixing old and new in the same config file. This seemed like the simplest approach that covered the most bases to me.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] keith-turner commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760405575


   I like the idea of centralizing handling of the old master props in the code and avoiding `AccumuloConfiguration.resolve` in lots of places. It would also be nice to avoid having lots of deprecated master props in the Accumulo Property ENUM if possible and centrally handle this special case in the code instead.  The centralized handling needs to ensure we have a concept of what the current master props are and freeze those, not mapping future new manager props to master props that never existed.
   
   @ctubbsii I like your proposal, it seems to imply that users would no longer be able to set master props in zookeeper.  Was that what you were 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [accumulo] keith-turner commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

keith-turner commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760524853


   > I'm not familiar enough with the property handling code to recognize if there's some fatal flaw in this idea, so please let me know if I missed something.
   
   The broad strokes you mentioned SGTM, if there is a fatal flaw it will certainly show up during implementation.  It seems like a good general change  that may make the prop deprecation code more maintainable, centralized, and consistent.
   
   Trying to think about how the proposal relates to this issue, I am trying to think of special cases that are unique to this large set of deprecated properties. One thing that comes to mind, is having a gigantic list of individually deprecated master props in the generated props docs.  That would be noisy and not very useful.
   
   Not sure if this [documetation](https://github.com/apache/accumulo-website#updating-property-documentation-for-releases) still works, it outlines how to render the [props docs](https://accumulo.apache.org/docs/2.x/configuration/server-properties).


----------------------------------------------------------------
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] ivakegg edited a comment on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

ivakegg edited a comment on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760441639


   I am thinking we may be too nice here.  I propose:
   1) Fail to startup if any of the master.* properties still exist in the site file with an informative message.
   2) Use the version upgrade mechanism to fix any overrides in zookeeper.


----------------------------------------------------------------
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] brianloss commented on issue #1640: Rename Accumulo master properties

Posted by GitBox <gi...@apache.org>.

brianloss commented on issue #1640:
URL: https://github.com/apache/accumulo/issues/1640#issuecomment-760491536

I was looking at `Property` and `AccumuloConfiguration` and was wondering what people think about something like the following... In `Property` the class has a member variable `replacedBy` that gets filled in if the `ReplacedBy` annotation is on a specific `Property` enum (that's done by `Property.precomputeAnnotations` which is invoked on each property enum value. What if we changed that to something like `deprecatedNames` so that a given property could have its deprecated names associated with it. I say names, plural, since we already have `master.bulk.rename.threadpool.size` deprecated and replaced by `master.rename.threadpool.size` so in my example the `deprecatedNames` for `manager.rename.threadpool.size`would be both `master.rename.threadpool.size` and `master.bulk.rename.threadpool.size`.

With this change, we could restructure `AccumuloConfiguration` so that `get` is no longer an abstract method (we'd rename the current abstract method and all of it's implementations to something else, call it `internalGet` for now) and then it would call the `internalGet` method with the property name. If that's not found, then it could try any `deprecatedNames` in turn. This would allow us to maintain the old and new property names and not have to use `AccumuloConfiguration.resolve`. In fact, any existing deprecated properties could take advantage of this approach too. At startup, we could still go through the site configuration and warn if we find any property names that are in the deprecated.

I'm not familiar enough with the property handling code to recognize if there's some fatal flaw in this idea, so please let me know if I missed something. Barring that, it seems like it could solve the concerns with the previously suggested approaches for this ticket and also provide a cleaner mechanism for any future property deprecations.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org