You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by googlielmo <gi...@git.apache.org> on 2015/11/04 15:45:17 UTC
[GitHub] incubator-brooklyn pull request: HTTP connection pooling for Brook...
GitHub user googlielmo opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1000
HTTP connection pooling for BrooklynApi clients
- introduce factories to create clients
- PoolingBrooklynApiFactory uses PoolingHttpClientConnectionManager
- DefaultBrooklynApiFactory replaces the old BrooklynApi constructors
using a new ApacheHttpClient4Executor for each client
- deprecate existing BrooklynApi constructors
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/googlielmo/incubator-brooklyn feature/client-connection-pool
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1000.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 #1000
----
commit 971c217d220986aece361b312b0793e4239d3d26
Author: googlielmo <gu...@cloudsoftcorp.com>
Date: 2015-11-04T13:30:05Z
HTTP connection pooling for BrooklynApi clients
- introduce factories to create clients
- PoolingBrooklynApiFactory uses PoolingHttpClientConnectionManager
- DefaultBrooklynApiFactory replaces the old BrooklynApi constructors
using a new ApacheHttpClient4Executor for each client
- deprecate existing BrooklynApi constructors
----
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r44033863
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -252,7 +386,7 @@ public AccessApi getAccessApi() {
}
/**
- * @deprecated Use {@link #getEntity(Response, GenericType)} instead.
+ * @deprecated since 0.9.0. Use {@link #getEntity(Response, GenericType)} instead.
--- End diff --
Whoops! Will fix. Thanks @sjcorbett!
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#issuecomment-154037402
Here's to another one thousand @grkvlt! :-)
---
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: HTTP connection pooling for Brook...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r44021956
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -252,7 +386,7 @@ public AccessApi getAccessApi() {
}
/**
- * @deprecated Use {@link #getEntity(Response, GenericType)} instead.
+ * @deprecated since 0.9.0. Use {@link #getEntity(Response, GenericType)} instead.
--- End diff --
This method has been deprecated since `0.8.0-incubating`.
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43898649
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
Could even be static methods on the factory class.
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43904828
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
But it's protected so no access to it?
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43898796
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -33,7 +33,7 @@
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.Credentials;
import org.apache.http.auth.UsernamePasswordCredentials;
-import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.impl.client.*;
--- End diff --
Will fix (bitten by "Optimize imports" in the IDE)
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43896654
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
Definitely possible, but then there would be only one static pool (versus one pool per factory).
How would one configure the static pool (max size etc.)?
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43899681
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
Good point. This one is deprecated but the next one is not. The idea was that if you want change implementation then probably you just have to write another factory (and use the next constructor) :)
---
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: HTTP connection pooling for Brook...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#issuecomment-154093627
LGTM. Test failure unrelated (/cc @geomacy it's the `HttpTest`)
---
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: HTTP connection pooling for Brook...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#issuecomment-154094942
Working on that now, per the conversation at https://github.com/apache/incubator-brooklyn/pull/994#issuecomment-154074295
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43898395
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
Should keep this one in case users need to fine-tune the executor creation - for example to use their own pooling implementation.
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43898472
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -33,7 +33,7 @@
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.Credentials;
import org.apache.http.auth.UsernamePasswordCredentials;
-import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.impl.client.*;
--- End diff --
The convention in Brooklyn is to list all used classes.
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43911237
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
Then probably it's best to reinstate the previous public constructor and remove the new one. The subclassing is certainly too complicated, especially if we don't follow the factory route.
---
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: HTTP connection pooling for Brook...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1000
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43904545
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
Actually, the threaded situation was the use case I wanted to tackle with that factory.
No problem in switching to a static pool *somewhere* (e.g., in a static factory like the one suggested by @neykov ) – I don't expect users to instantiate more than one factory in any case :-)
---
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: HTTP connection pooling for Brook...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43900491
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
why would i want to create two `BrooklynApi` instances sharing a pool? feels like that use case isn't compelling enough to justify having a factory. if i wanted pool-sharing behaviour i'd use the same `BrooklynApi` instance. (if that's not allowed for thread reasons then i'd rather have a wrapper which does make it thread safe, as opposed to having to keep a handle on a factory instance.)
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43911912
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
I heartily agree the factory is just a means to an end, in this case easy configurability (independence from any particular JAX-RS implementation would be another, but it's not the point of this PR).
We can achieve that goal in many ways! I'll do an alternative implementation with the static pool as @ahgittin suggested. ^_^
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43906767
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
Yup, it's a little too protective there. What if we change to `protected` and let the user subclass the `BrooklynApi` to provide an alternative implementation?
---
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: HTTP connection pooling for Brook...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#issuecomment-153832762
Congratulations on the **one thousandth** pull request for Brooklyn @googlielmo :cake: :cake: :cake:
---
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: HTTP connection pooling for Brook...
Posted by googlielmo <gi...@git.apache.org>.
Github user googlielmo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#issuecomment-154040776
Latest commit addresses comments.
Each `BrooklynApi` instance now maintains a private pool of HTTP connections.
The `newInstance()` static factory methods now create `BrooklynApi`'s using the connection pool.
A custom `ClientExecutor` (for cases in which the default pool implementation is not applicable) can be provided by calling a dedicated constructor or by overriding `getClientExecutor()`
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43908770
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApi.java ---
@@ -114,11 +150,23 @@ public BrooklynApi(String endpoint, @Nullable Credentials credentials) {
}
}
+ /**
+ * @deprecated since 0.9.0. Use {@link BrooklynApiFactory#getBrooklynApi(java.net.URL, String, String)} instead
+ * @see org.apache.brooklyn.rest.client.DefaultBrooklynApiFactory
+ * @see org.apache.brooklyn.rest.client.PoolingBrooklynApiFactory
+ */
+ @Deprecated
--- End diff --
Sounds a bit too involved just to construct the object. When in doubt I try to follow the "Keep it simple" moto. Nothing wrong with using it without a factory. The factory just does some sugaring over the parameters.
---
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: HTTP connection pooling for Brook...
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/1000#discussion_r43898321
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
Don't think we need the non-pooling variant.
@googlielmo makes a good point re configuration. I think it's a good balance if we keep the DefaultBrooklynApiFactory and remove the interface and the default implementation.
+1 for having non-username-password overload.
---
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: HTTP connection pooling for Brook...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43980104
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
still not convinced the factory gives us anything over a static `BrooklynApi.newInstance(URI)`
---
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: HTTP connection pooling for Brook...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1000#discussion_r43895360
--- Diff: usage/rest-client/src/main/java/org/apache/brooklyn/rest/client/BrooklynApiFactory.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.rest.client;
+
+import java.net.URL;
+
+/**
+ * Creates {@link BrooklynApi} instances.
+ */
+public interface BrooklynApiFactory {
--- End diff --
does this class pull its weight, as opposed to having static methods:
BrooklynApi.newInstance(URL endpoint);
BrooklynApi.newInstance(URL endpoint, String username, String password);
BrooklynApi.newInstance(URL endpoint, ClientExecutor clientExecutor);
with a comment to the effect that the first two return pooling executors (ie when would someone not want that?).
@neykov wdyt?
---
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.
---