Skip to content
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

Jump to bottom

common/ceph_context: Show clear message if all features are enabled #12676

Merged
merged 1 commit into from Dec 30, 2016
Merged

common/ceph_context: Show clear message if all features are enabled #12676

merged 1 commit into from Dec 30, 2016

Conversation

chendave
Copy link
Contributor

@chendave chendave commented Dec 27, 2016 •
edited by tchaikov

It's saying the following dangerous and experimental featuresare enabled
while the value is "*", it's not quite clear what's "*" stands for.

The patch update the message so that it's more clear when "*" is used in
place.

Signed-off-by: Dave Chen wei.d.chen@intel.com

tchaikov
tchaikov reviewed Dec 27, 2016
View reviewed changes
src/common/ceph_context.cc
<< cct->_experimental_features << dendl;
if (!cct->_experimental_features.empty()) {
std::set <std::string>::iterator it = cct->_experimental_features.begin();
if (cct->_experimental_features.size() == 1 && *it == "*") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be more consistent by putting

if (cct->_experimental_features.count("*")) {
  lderr(cct) << "WARNING: all dangerous and experimental features are enabled." << dendl;
} else {
  ...
}

instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, thanks! will update shortly according to the suggestion!

@chendave
Copy link
Contributor Author

@tchaikov updated, pls take another look at your convenience, thanks!

tchaikov
tchaikov reviewed Dec 27, 2016
View reviewed changes
src/common/ceph_context.cc
if (!cct->_experimental_features.empty()) {
if (cct->_experimental_features.count("*")) {
lderr(cct) << "WARNING: all dangerous and experimental features are enabled." << dendl;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, could you move the else { up to the previous line, so it looks like

} else {
 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will do, it is look better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov , updated, thanks for the review and the comments!

@chendave
common/ceph_context: Show clear message if all features are enabled
091547d 
It's saying the following dangerous and experimental featuresare enabled
while the value is "*", it's not quite clear what's "*" stands for.

The patch updates the message so that it's more clear when "*" is used
in place.

Signed-off-by: Dave Chen <wei.d.chen@intel.com>
@chendave
Copy link
Contributor Author

How can I trigger the CI to run the build again? looks like it's irrelevant with the change here.

@xiexingguo
Copy link
Member

How can I trigger the CI to run the build again?

retest this please

looks like it's irrelevant with the change here.

yeah

@chendave
Copy link
Contributor Author

@xiexingguo , thanks for help, looks like it's the magic of "retest" keyword!

@chendave
Copy link
Contributor Author

retest this please

@tchaikov
Copy link
Contributor

retest this please

Collecting pip>=7.0
  File was already downloaded /home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-detect-init/wheelhouse-wip/pip-9.0.1-py2.py3-none-any.whl
Collecting wheel>=0.24
  File was already downloaded /home/jenkins-build/build/workspace/ceph-pull-requests/src/ceph-detect-init/wheelhouse-wip/wheel-0.29.0-py2.py3-none-any.whl
Skipping setuptools, due to already being wheel.
Skipping pip, due to already being wheel.
Skipping wheel, due to already being wheel.
Collecting coverage>=3.6 (from -r test-requirements.txt (line 1))
  Using cached coverage-4.3.tar.gz
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-noa5883l/coverage/setup.py", line 50, in <module>
        paras = contributors.read().split("\n\n")
      File "/home/jenkins-build/build/workspace/ceph-pull-requests/install-deps-python3/lib/python3.4/encodings/ascii.py", line 26, in decode
        return codecs.ascii_decode(input, self.errors)[0]
    UnicodeDecodeError: 'ascii' codec can't decode byte 0xc4 in position 846: ordinal not in range(128)
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-noa5883l/coverage/
Build step 'Execute shell' marked build as failure
[PostBuildScript] - Execution post build scripts.

@chendave
Copy link
Contributor Author

retest this please

1 similar comment
@chendave
Copy link
Contributor Author

retest this please

@tchaikov tchaikov self-assigned this Dec 29, 2016
@liewegas liewegas merged commit 9840537 into ceph:master Dec 30, 2016
@liuchang0812
Copy link
Contributor

By the way, could we provide a option to close this warn?

@chendave
Copy link
Contributor Author

chendave commented Jan 3, 2017

@liuchang0812 sound like a good idea, I will take a look at this, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

Assignees

@tchaikov tchaikov

Labels
common feature needs-qa wip-sage-testing
Projects
None yet
Milestone
No milestone
5 participants
@chendave @xiexingguo @tchaikov @liuchang0812 @liewegas