-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP : Ranger HTTP client implementations #41
Conversation
…erviceprovider and servicehub package structures, each workflow has a containment 2. Added an implementation for http node data sink and registration
Added a dummy project structure for ranger-server
…, finderhub and health
…t custom criteria could be passed to the shardSelector which can help in shardSelection
… one for mapBasedRegistry and one for listBasedServiceRegistry b) Tests added for the broken down criteria across all the modules
…y purposes and to manage the lifecycle within
…e you may want to build
…valMs to have it consistent with the rest of the code
…ere acquired. 3. Addee license on the missing files. 4. Sonar fixes
|
||
package com.flipkart.ranger.core.finder.unsharded; | ||
public class Constants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce a private Constructor
@@ -13,17 +13,10 @@ | |||
* See the License for the specific language governing permissions and | |||
* limitations under the License. | |||
*/ | |||
package com.flipkart.ranger.client; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it showing up as a rename of the file?
serviceNodes.add(new ServiceNode<>("localhost-2", 9001, TestNodeData.builder().nodeId(2).build())); | ||
serviceNodes.add(new ServiceNode<>("localhost-3", 9002, TestNodeData.builder().nodeId(3).build())); | ||
ServiceNode<TestNodeData> select = roundRobinSelector.select(serviceNodes); | ||
Assert.assertTrue(select.getHost().equalsIgnoreCase("localhost-2")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why equalsIgnoreCase
etc? use Assert.assertEquals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in all places
@@ -39,7 +48,7 @@ public HttpNodeDataStoreConnector( | |||
.connectionPool(new ConnectionPool(1, 30, TimeUnit.SECONDS)) | |||
.build(); | |||
this.config = config; | |||
this.mapper = mapper; | |||
this.mapper = null != mapper ? mapper : new ObjectMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not create object mapper inside constructor
@Builder | ||
public class ServiceDataSourceResponse { | ||
private boolean success; | ||
private List<Service> data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an error message here
log.error("No data returned from server: " + httpUrl); | ||
|
||
if (null != serviceNodesResponse && null != serviceNodesResponse.getData() && | ||
!serviceNodesResponse.getData().isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is error message in the response, log that
|
||
@Test | ||
public void testProvider() throws Exception { | ||
TestNodeData nm5NodeData = TestNodeData.builder().farmId("nm5").build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val
@Test | ||
public void testProvider() throws Exception { | ||
TestNodeData nm5NodeData = TestNodeData.builder().farmId("nm5").build(); | ||
ServiceNode<TestNodeData> testNode = new ServiceNode<>("localhost-1", 80, nm5NodeData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val .. make things consistent
@GET | ||
@Path("/nodes/{namespace}/{serviceName}") | ||
@Timed | ||
@ExceptionMetered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do @metered and removed the above two anotations
|
||
private final RangerConfiguration rangerConfiguration; | ||
|
||
private final UnshardedRangerZKHubClient<T, C> zkHubClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to rethink this. Ideally this should be abstract here, so that the server can, if it wants, add some in-memory cached decorator to the hub
Apologies for the massive-ish PR. I understand the incredible burden of having to go through it.
Split the T of serviceNode and Criteria and that contributed to one ridiculous spur of changes. Didn't make sense breaking that in parts, and the tests to support the same have been added too - again didn't make sense to submit code and test in parts.
The additional ones are the additional modules, ranger-http-client, ranger-zk-client, and ranger-server (Really small interface classes). The split of criteria and T of serviceNode has bloated the PR up.
This is an extension of, #31
Why separate T and Criteria
serviceNode
.Major LLD Callouts
ShardSelector<T, C extends Criteria<T>> implements ShardSelector<T, C, R extends Registry<T>>
orShardSelector<T> implements ShardSelector<T, Criteria<T>, R extends Registry<T>>
. Chose the former to keep the Criteria implementation fluid and to be able to have clients define their shardSelector within the bound of any Criteria implementation.Changelist