[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[zuyu]

Github user zuyu commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
We need to sync up with the master branch before merge.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
After @hbdeshmukh and I's code review, we created:
* QUICKSTEP-90
* QUICKSTEP-91
in response to some of the issues we found. @zuyu 90 will address the async 
upgrade.

Additionally, we changed the locks in `NetworkIO.hpp` to be quickstep 
locks, not `std` locks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[zuyu]

Github user zuyu commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
I mean, if this class is always a base class, move the constructor in
protected session.
On Mon, Apr 24, 2017 at 12:58 PM Marc S  wrote:

> *@cramja* commented on this pull request.
> --
>
> In cli/IOInterface.hpp
> 

> :
>
> > + * specific language governing permissions and limitations
> + * under the License.
> + **/
> +
> +#ifndef QUICKSTEP_CLI_IO_INTERFACE_HPP_
> +#define QUICKSTEP_CLI_IO_INTERFACE_HPP_
> +
> +#include 
> +
> +/**
> + * Virtual base defines a generic, file-based interface around IO.
> + */
> +class IOInterface {
> + public:
> +  IOInterface() {}
> +
>
> I removed the constructor but ran into a compiler error:
>
> 
/Users/cramja/workspace/quickstep/incubator-quickstep/cli/NetworkIO.cpp:50:12: 
error: constructor for 'quickstep::NetworkIO' must explicitly initialize the 
base class 'quickstep::IOInterface' which does not have a default constructor
> NetworkIO::NetworkIO() {
>^
> 
/Users/cramja/workspace/quickstep/incubator-quickstep/cli/IOInterface.hpp:62:7: 
note: 'quickstep::IOInterface' declared here
> class IOInterface {
>   ^
>
> On doing the suggestion in the output, that too gave a compiler error.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> 
,
> or mute the thread
> 

> .
>



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
@zuyu Thank you for review. I addressed the minor stuff and will look into 
Async when I have more time this coming week.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
I should also mention that @hbdeshmukh and I had a discussion yesterday 
about refactoring the CLI main method. I was unhappy with how cluttered seeming 
it has become, and this PR only makes it slightly more cluttered. I came up 
with a solution which I have been working in a WIP branch. It uses RIIA to 
close down any IO-related state (eg network connections) by creating an 
`IOHandle` for every cli user interaction. Because I think this change is a big 
improvement over, and complimentary to the change which is present here, I will 
submit it as a commit on top of this commit in the PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
@zuyu Thank you for review. Regarding the question about mutexes and 
cond_var in the `NetworkCliServiceImpl`, we disallow multiple gRPC threads from 
submitting requests at the same time.

As for the suggestion, I think it's okay to split those up. I didn't do 
this in the draft because it creates very small test files.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-quickstep issue #232: QUICKSTEP-87 Adds network cli interface.

[cramja]

Github user cramja commented on the issue:

https://github.com/apache/incubator-quickstep/pull/232
  
Thanks @hbdeshmukh , fixed validate_cmakelists issue.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---