-
Notifications
You must be signed in to change notification settings - Fork 468
moved cluster classes into miniClusterImpl #5897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cluster stuff is hard to move right now, because it's so tightly integrated into our test stuff. The AccumuloCluster, and related classes like ClusterUsers and ClusterControl, are part of an abstraction above minicluster that intended to provide basic programmatic cluster management controls over any Accumulo cluster. Since it's not part of minicluster, it shouldn't be moved into the minicluster package. They exist so that we could support running tests against a separate cluster instance called "Standalone" (which also isn't part of minicluster and shouldn't be moved into it). But, we don't really support running our test suite against a separate standalone instance anymore (that feature hasn't been maintained for a long time). The classes still exist because it's hard to rewrite all of our IT base classes, because one touch seems to propagate like wildfire.
I would suggest focusing on moving other stuff for now, and leaving the cluster stuff for later. Ultimately, it would probably make sense to remove the cluster management tooling entirely from the minicluster module, and perhaps put it in its own project or module, if we keep it at all. Since we don't support the standalone ITs anymore, it may not make sense to keep it at all. But that's outside the scope of this PR.
| ==== | ||
| Licensed to the Apache Software Foundation (ASF) under one | ||
| or more contributor license agreements. See the NOTICE file | ||
| distributed with this work for additional information | ||
| regarding copyright ownership. The ASF licenses this file | ||
| to you under the Apache License, Version 2.0 (the | ||
| "License"); you may not use this file except in compliance | ||
| with the License. You may obtain a copy of the License at | ||
|
|
||
| https://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, | ||
| software distributed under the License is distributed on an | ||
| "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| KIND, either express or implied. See the License for the | ||
| specific language governing permissions and limitations | ||
| under the License. | ||
| ==== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header will break. The exception in the pom.xml needs to be updated instead of adding a license header to this data file.
Other exceptions/exclusions statements in the pom.xml files should be checked and updated, as needed, for any other moved files as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to wait on the files in harness as well since they also since they also seem to affect a lot of our test?
Closes issue #5867
After a discussion this pr is focusing on updating all the resource files with the correct package naming convention. The following resources where updated:
The files currently untouched are located under test/.../harness, but since the files in cluster are to be removed I suggest waiting on updating these until cluster is removed.