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

ceph.in: Fix couple of minor issues on the messages #12797

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
2 participants
@chendave
Copy link
Contributor

chendave commented Jan 5, 2017

  • e.class.name is the name of class, use the error message assembled
    by function: make_ex instead.
  • repr(e) will also print the name of class, end use needn't care about
    the class name but the useful error message.
  • General clean up.

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

@chendave

This comment has been minimized.

Copy link
Contributor Author

chendave commented Jan 5, 2017

retest this please

@chendave chendave force-pushed the chendave:wip_error_message branch from c7b8050 to 99875bf Jan 6, 2017

@chendave

This comment has been minimized.

Copy link
Contributor Author

chendave commented Jan 6, 2017

looks like some testcase failed due to the change, will look into it.

@@ -732,10 +731,10 @@ def main():
print('Cluster connection aborted', file=sys.stderr)
return 1
except rados.PermissionDeniedError as e:
print('Error connecting to cluster: {0}'.format(e.__class__.__name__), file=sys.stderr)
print(str(e), file=sys.stderr)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 6, 2017

Contributor

you removed the "Error connecting to cluster: " from the error message. is this intentional?

This comment has been minimized.

Copy link
@chendave

chendave Jan 6, 2017

Author Contributor

@tchaikov thanks for the comments! yep it's intentional. instance "e" has already has the error message, the message is assembled here.
https://github.com/ceph/ceph/blob/master/src/pybind/rados/rados.pyx#L408-L422

The original code will say:

  • Error connecting to cluster: Error (Error is the name of class given by e.class.name)
  • If we dont remove "Error connecting to cluster: ", it will say as this:
    Error connecting to cluster: error connecting to cluster: error code xx
    As you can see, it's quite redundant, all error message has been included in the instance of "e", we can just pull the message out.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 6, 2017

Contributor

thanks, got it. so it will put

Error connecting to cluster: error connecting to the cluster: error code $error_code

without this change

return errno.EACCES
except Exception as e:
print('Error connecting to cluster: {0}'.format(e.__class__.__name__), file=sys.stderr)
print(str(e), file=sys.stderr)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 6, 2017

Contributor

ditto.

This comment has been minimized.

Copy link
@chendave

chendave Jan 6, 2017

Author Contributor

same here.

src/ceph.in Outdated
@@ -715,7 +714,7 @@ def main():
timeout = 5

hdr('Monitor commands:')
print('[Contacting monitor, timeout after %d seconds]' % timeout)
print('[Contacting monitor, timeout after %d seconds]' % timeout, file=sys.stderr)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 6, 2017

Contributor

why would we print this info to stderr? it's but informational string, not error message.

This comment has been minimized.

Copy link
@chendave

chendave Jan 6, 2017

Author Contributor

I just want it consistent with others, but I agree with you there is no need to print it out to stderr, will address it shortly.

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jan 6, 2017

also you might want to change the prefix of "script: " to "ceph.in: ". as "script" is too general in this context.

@chendave chendave force-pushed the chendave:wip_error_message branch from 99875bf to 1635c4c Jan 6, 2017

@tchaikov tchaikov changed the title script: Fix couple of minor issues on the messages ceph.in: Fix couple of minor issues on the messages Jan 9, 2017

@tchaikov tchaikov added the needs-qa label Feb 8, 2017

@tchaikov tchaikov self-assigned this Feb 8, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 8, 2017

retest this please (jenkins log gone)

@chendave

This comment has been minimized.

Copy link
Contributor Author

chendave commented Feb 8, 2017

@tchaikov , thanks for visiting it again, looks like it failed some testcases, just cannot figure out the reason, will look into this again, thanks!

ceph.in: Fix couple of minor issues on the messages
- e.__class__.name is the name of class, use the error message assembled
by function: make_ex instead.
- repr(e) will also print the name of class, end user needn't care about
the class name but the useful error message.
- General clean up.

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

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 8, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/qa/workunits/ceph-helpers.sh: line 381: /home/jenkins-build/build/workspace/ceph-pull-requests/build/bin/ceph: Permission denied
/home/jenkins-build/build/workspace/ceph-pull-requests/qa/workunits/ceph-helpers.sh:381: run_mon:  return 1

https://jenkins.ceph.com/job/ceph-pull-requests/18109/consoleFull

@chendave you chmod'ed ceph.in, please undo this change. see https://github.com/ceph/ceph/pull/12797/files

src/ceph.in 100755 → 100644
@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 8, 2017

also you might want to change the prefix of "script: " to "ceph.in: ". as "script" is too general in this context.

@chendave, hey. better address this also. "git amend" will do the trick.

@chendave chendave force-pushed the chendave:wip_error_message branch from 1635c4c to b2b3a3f Feb 8, 2017

@chendave

This comment has been minimized.

Copy link
Contributor Author

chendave commented Feb 8, 2017

@tchaikov many thanks for your help!

@tchaikov tchaikov merged commit 92e51fd into ceph:master Feb 17, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.