GEODE-4517: Remove CacheFactory.getAnyInstance call from CliUtil #1488
Conversation
There was a problem hiding this comment.
At one point I wanted to get rid of the CliUtil completely. Here are the steps I had in mind:
- get rid of the usage of CliUtils other than gfsh commands. I think part of your commits are doing exactly this.
2). Since all gfsh commands extends GfshCommand, have GfshCommand do the real work, instead of having GfshCommand calling CliUtils. This way we get rid of CliUtils.
3). Now GfshCommand needs to get ahold of cache. This is done by CommandManager, when initializing all the command, we can inject cache into each command.
| @@ -45,10 +45,9 @@ | |||
|
|
|||
| private Logger logger; | |||
|
|
|||
| private LogWrapper() { | |||
| private LogWrapper(Cache cache) { | |||
There was a problem hiding this comment.
I am not sure why LogWrapper needs a cache instance. LogWrapper is used by gfsh logging. And on server/locators, we are solely using LogService now. Probably check with Kirk and see if we can get rid of the the usage of cache inside LogWrapper.
There was a problem hiding this comment.
@kirklund Any insight? I looks like we're grabbing the cache's log handler so that things appear in the cache's logs? I would imagine there are places where the LogWrapper can't be replaced by the LogService.getLogger(), or else it would've been part of that initial rework.
[edit:] I think I was getting LogWrapper and LogWriter mixed up...
There was a problem hiding this comment.
@metatype Someone mentioned elsewhere that you might have insight on how the java.util loggers and the cache's handlers interact?
There was a problem hiding this comment.
Maybe we get delete LogWrapper and change GFSH code to use Log4J 2? You might want to try that in a branch and run as many tests as you can. Just change any GFSH class that uses JULI to instead get a Logger from LogService.
PurelyApplied
force-pushed
the
geode-4517
branch
from
941a45e
to
886cc0f
Compare
|
@jinmeiliao @kirklund For what it's worth, some manual testing shows minor but probably trivial differences in replacing the where the I don't know what the |
|
Precheckin green. |
PurelyApplied
requested review from
jdeppe-pivotal,
sboorlagadda and
pdxrunner
* CliUtil.getCacheIfExists now takes supplier, but will squelch error and return null as before. * LogWriter.getInstance now explicitly takes a Cache, to hook into the cache's LogWriter. * GfshExecutionStrategy adopts the Gfsh shell's LogWriter, since the shell will necessarily have * * finished instantiating the LogWriter's singleton before GfshExecutionStrategy can instantiate. * In some instances, the LogWriter has been replaced with the Log4j LogService logger. * Launcher explicitly passes a null Cache, since it does not yet exist. * Other minor improvements: visibility, typo corrections, etc.
PurelyApplied
force-pushed
the
geode-4517
branch
from
70b22da
to
2b73333
Compare
|
Accidentally pushed the branch rebased on the newest |
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.