You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2015/06/02 13:44:47 UTC
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/672
Don't spin BrooklynEntityMirror forever on failure
* Fail on non-{2xx, 403} response received
* Fail on BrooklynNode failure
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn mirror-fail
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/672.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #672
----
commit 54b74c8015bf9c7c680daf6f3391c32c456b7ea4
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-06-02T11:43:57Z
Don't spin BrooklynEntityMirror forever on failure
* Fail on non-{2xx, 403} response received
* Fail on BrooklynNode failure
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/672#discussion_r31716404
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/EntityHttpClientImpl.java ---
@@ -77,6 +81,12 @@ protected EntityHttpClientImpl(Entity entity, ConfigKey<?> urlConfig) {
return builder;
}
+ @Override
+ public EntityHttpClient responseSuccess(Predicate<Integer> responseSuccess) {
+ responseSuccess = checkNotNull(responseSuccess, "responseSuccess");
--- End diff --
ouch, good find!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/672#issuecomment-108877325
Looks good. One thing to fix, and another comment to see what you think of. Then good to 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/672#discussion_r31715986
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/EntityHttpClientImpl.java ---
@@ -77,6 +81,12 @@ protected EntityHttpClientImpl(Entity entity, ConfigKey<?> urlConfig) {
return builder;
}
+ @Override
+ public EntityHttpClient responseSuccess(Predicate<Integer> responseSuccess) {
+ responseSuccess = checkNotNull(responseSuccess, "responseSuccess");
--- End diff --
should be `this.responseSuccess = ...`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/672#issuecomment-108884524
Following the guava conventions seems like a good idea. Addressed comments.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/672
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Don't spin BrooklynEntityMirror f...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/672#discussion_r31715825
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/EntityHttpClient.java ---
@@ -23,10 +23,29 @@
import brooklyn.util.http.HttpTool;
import brooklyn.util.http.HttpToolResponse;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.Range;
+
/**
* Helpful methods for making HTTP requests to {@link BrooklynNode} entities.
*/
public interface EntityHttpClient {
+ interface ResponseCodePredicates {
+ static class ResponseCodeHealthyPredicate implements Predicate<Integer> {
+ @Override
+ public boolean apply(Integer input) {
+ return HttpTool.isStatusCodeHealthy(input);
+ }
+ }
+ Predicate<Integer> INFORMATIONAL = Range.closed(100, 199);
--- End diff --
I have a personal preference for methods rather than static constants, which gives us future flexibility. That follows the guava convention, such as the method `Predicates.alwaysTrue()`. In this case I can't see why we'd ever change it, but I'd prefer us sticking to the general guava pattern.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---