You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/04/23 17:28:33 UTC
[GitHub] [cassandra-in-jvm-dtest-api] dcapwell opened a new pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
dcapwell opened a new pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-in-jvm-dtest-api] dcapwell commented on a change in pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10#discussion_r413993445
##########
File path: src/main/java/org/apache/cassandra/distributed/api/CompleteQueryResult.java
##########
@@ -0,0 +1,141 @@
+/*
Review comment:
this isn't a new file but renamed `QueryResult` to this and made `QueryResult` a interface.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-in-jvm-dtest-api] dcapwell commented on pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
Posted by GitBox <gi...@apache.org>.
dcapwell commented on pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10#issuecomment-619431181
> should we move classes to shared package and leave only interfaces in api?
I don't agree with that if the class is part of the API. The main one I see in your commit is `Row` which to me is part of the API. Anything a interface returns is part of the API to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-in-jvm-dtest-api] dcapwell edited a comment on pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
Posted by GitBox <gi...@apache.org>.
dcapwell edited a comment on pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10#issuecomment-619431181
> should we move classes to shared package and leave only interfaces in api?
I don't agree with that if the class is part of the API; anything a interface returns is part of the API to me. For this reason I didn't move the classes you did to shared, I only see "shared" really as utility classes (even QueryResults is part of the public API to me)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-in-jvm-dtest-api] ifesdjeen commented on pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
Posted by GitBox <gi...@apache.org>.
ifesdjeen commented on pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10#issuecomment-619106594
I've pushed several suggestions here (thought might be quicker this way): https://github.com/apache/cassandra-in-jvm-dtest-api/compare/master...ifesdjeen:feature/allApiUseQueryResult?expand=1
Caan be summarised as:
* should we move _classes_ to `shared` package and leave only interfaces in `api`?
* remove `get` from some methods
* rename `Complete` to `SImple` result
I think these API changes will also help us with paged query coordinator switch in the future!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-in-jvm-dtest-api] dcapwell commented on pull request #10: CASSANDRA-15756 in-jvm dtest IInstance and ICoordinator should use QueryResult as the base API
Posted by GitBox <gi...@apache.org>.
dcapwell commented on pull request #10:
URL: https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10#issuecomment-619433783
@ifesdjeen https://github.com/apache/cassandra-in-jvm-dtest-api/pull/10/commits/ca592b43531c7ce1f5df1d252c78de63b456c952. this is based off your feedback. the only real diff is I didn't move anything to shared.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org