Skip to content

Conversation

minishrink
Copy link
Contributor

  • Added an assertion for SR.probe as it is currently only safe with clustering enabled.
  • Wrapped Xapi_sr.call_probe (called by both SR.probe and SR.probe_ext) in a clustering lock in case the SR is in use elsewhere to prevent database inconsistency
  • Removed local opens of Storage_interface and Storage_access

Signed-off-by: Akanksha Mathur akanksha.mathur@citrix.com

Copy link
Contributor

@mseri mseri left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me but I'd like somebody with more context to have a look. Ping @edwintorok @gaborigloi

@coveralls
Copy link

coveralls commented Apr 4, 2018

Coverage Status

Coverage decreased (-0.02%) to 20.027% when pulling cd7bd61 on minishrink:feature/REQ477/check_probe into e6e7dc2 on xapi-project:feature/REQ477/probe.

@mseri
Copy link
Contributor

mseri commented Apr 4, 2018

Why coveralls keeps messing the count?

@minishrink
Copy link
Contributor Author

I don't know how coveralls works, but is it possible that local opens contribute significantly?

@mseri
Copy link
Contributor

mseri commented Apr 4, 2018

I don't think that's the issue. We have seen it happen in multiple PRs lately, even in ones where we ehere deleting lots of code

Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

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

This looks good, but please submit the PR to the feature/REQ477/probe branch where we collect the changes for probe_ext API, otherwise we will have conflicts as it touches the same code.

@minishrink minishrink changed the base branch from master to feature/REQ477/probe April 4, 2018 15:08
@minishrink minishrink force-pushed the feature/REQ477/check_probe branch from 2ae796b to dc35b09 Compare April 4, 2018 15:12
@edwintorok edwintorok force-pushed the feature/REQ477/probe branch 2 times, most recently from f304da3 to e6e7dc2 Compare April 4, 2018 16:38
@edwintorok
Copy link
Contributor

@minishrink please rebase there are too many commits in the PR, which means you probably have some duplicate commits that are already on the probe branch but with a different hash.

@minishrink minishrink force-pushed the feature/REQ477/check_probe branch from dc35b09 to cd7bd61 Compare April 5, 2018 08:08
@edwintorok edwintorok force-pushed the feature/REQ477/probe branch from e6e7dc2 to 42f3b9f Compare April 5, 2018 08:59
Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
@minishrink minishrink force-pushed the feature/REQ477/check_probe branch from cd7bd61 to ccfee3c Compare April 5, 2018 09:05
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now

@edwintorok edwintorok merged commit 1c97905 into xapi-project:feature/REQ477/probe Apr 5, 2018
@minishrink minishrink deleted the feature/REQ477/check_probe branch April 5, 2018 09:08
edwintorok pushed a commit that referenced this pull request Apr 5, 2018
…3542)

Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants