Redis and Minor Optimization #54
Conversation
devvsda
left a comment
devvsda
left a comment
There was a problem hiding this comment.
Please address given comments.
| log.info(String.format("Processing register endpoint for %s", registerEndpointRequest)); | ||
|
|
||
| Integer endpointId = clientRegisterationService.registerEndpoint(registerEndpointRequest); | ||
| String endpointId = clientRegisterationService.registerEndpoint(registerEndpointRequest); |
There was a problem hiding this comment.
If converting to string, then use UUID, and change .sql file as well.
| int clientId = clientDetails.getClientId(); | ||
|
|
||
|
|
||
| EndpointDetails endpointDetails = registerationDao.getEndpointDetails(clientId, endpointName); |
There was a problem hiding this comment.
+1 for moving this to DocumentDB from MySQL.
| // validate. | ||
| // Get ClientId , and EndpointId. | ||
| ClientDetails clientDetails = registerationDao.getClientDetails(executeWorkflowRequest.getClientName()); | ||
| ClientDetails clientDetails = executeWorkflowServiceHelper.getClientDetailsAndSave(executeWorkflowRequest.getClientName()); |
| private RabbitMQOperation rabbitMQOperation; | ||
|
|
||
| @Inject | ||
| private RedisCache redisCache; |
There was a problem hiding this comment.
Ideally any caching service should not be composite in any class. It will create problem, if we want to move away from Redis in future.
Its always recommended to put all caching logic in one class, and inject that class in required classes.
We can take it later, but just mark it as TODO.
| * @return EndpointDetails | ||
| */ | ||
| public ClientDetails getClientDetailsAndSave(String clientName) { | ||
| String cacheKey = clientName; |
| dbObjectInput.append(ExecutionDocumentConstants.Fields.ENDPOINT_NAME, endpointDetails.getEndpointName()); | ||
| dbObjectInput.append(ExecutionDocumentConstants.Fields.ENDPOINT_DETAILS, JSONLoader.stringify(endpointDetails)); | ||
| collection.insertOne(dbObjectInput); | ||
| log.debug(String.format("EndPoint details are inserted : %s inserted successfully in \n collection : %s and db : %s", endpointDetails, "graph_endpoint", "shepherd_graph_endpoint")); |
There was a problem hiding this comment.
Remove all the hardcoded strings. Please make this habit from day#1. Use root level constants file for the same.
| } | ||
| } | ||
| } catch (MongoWriteException e) { | ||
| log.error(e.getMessage(), e); |
There was a problem hiding this comment.
improve logging. please use logging written as reference. Try to use single standard throughtout the service.
| log.info(String.format("created a new Connection Id %s on %s:%d/%s",consumerConnection.getId(),hostName,portNumber,virtualHost)); | ||
| }else{ | ||
| log.warn("Connection is already established, No new connection created"); | ||
| log.warn("Connection is already established, Using already present consumer connection"); |
| } catch (Exception e) { | ||
| log.error("Node-message processing failed.", e); | ||
| // TODO : NACK | ||
| channel.basicNack(deliveryTag,false,true); |
There was a problem hiding this comment.
Did you tested this NAck ? Is it pushing back to queue ? Also, we need to check feature like visibility timeout in rabbit. Reference : https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-visibility-timeout.html
If there is no such feature in Rabbit, then it may harm Shepherd threads. We need to come with some approach for the same.
| import java.security.NoSuchAlgorithmException; | ||
| import java.util.concurrent.TimeoutException; | ||
|
|
||
| public class RedisCacheTest { |
3 participants
No description provided.