Conversation
Ckobar
left a comment
Ckobar
left a comment
There was a problem hiding this comment.
Example test command:
$ curl -H "Accept: application/json" -H "Content-type: application/json" -X POST -d '{"id":5, "name":"yyy3", "description":"rr", "tags":["t1","t2"], "script":"script", "parameters":"parameters"}' http://localhost:8080/create/nodes
curl -H "Accept: application/json" -H "Content-type: application/json" -X POST -d '{"listOfNameNodes":["yyy1","yyy4"]}' http://localhost:8080/create/graphs
avplatonov
left a comment
avplatonov
left a comment
There was a problem hiding this comment.
Thanks for PR. Please read my comment carefully.
Among general comments: try to use interfaces, dependency injections and mock objects for stubs. This practice will help you on next stages of project.
| public String service(@RequestBody NameNodesToGraph nameNodesToGraph) throws IOException { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| //listOfNodes.add(nameNodesToGraph); | ||
| if(createGraph(nameNodesToGraph).isEmpty()) |
There was a problem hiding this comment.
I think you should use createGraph result for echo-answer for this stub
In other cases I don't understand why you use createGraph in this method
There was a problem hiding this comment.
Creating graph by uuid nodes (before - name) why not?
|
|
||
| private List<Node> createGraph(NameNodesToGraph nameNodesToGraph) { | ||
| List<Node> listOfGraphsLocal = new ArrayList<>(); | ||
| for (String graph : nameNodesToGraph.listOfNameNodes) { |
There was a problem hiding this comment.
Why you use "graph"-name for variable?
You iterate over node names, yeah?
| List<Node> listOfGraphsLocal = new ArrayList<>(); | ||
| for (String graph : nameNodesToGraph.listOfNameNodes) { | ||
| boolean containsNode = false; | ||
| for (Node node : listOfNodes) { |
There was a problem hiding this comment.
What is listOfNodes?
Static list?
First of all. Why you use List instead of Set for this purpose. In case of Set you will not use iterate over all known nodes.
Secondly. You should use some abstraction over known node templates like DB (it may be implemented as list internally, but interface hide realization) and use method like "isKnownNode"
And finally, try to avoid using of static objects/methods and others. Spring framework supports dependency injections.
There was a problem hiding this comment.
Set done
DB emulation done (I think so)
But, how it help me not use iterate all nodes? Search by name|tags
And m.b. need make Map?
| listOfGraphsLocal.add(node); | ||
| } | ||
| } | ||
| if(!containsNode) |
There was a problem hiding this comment.
If all of node names don't exists in templates DB then this list will be empty without such check.
Nevertheless, if at least one node from user's request doesn't exist in DB then you should return empty list or throw IllegalArgumentException than will return HTTP 400 to user with error description.
| if(!containsNode) | ||
| return new ArrayList<>(); | ||
| } | ||
| listOfGraphs.add(listOfGraphsLocal); |
There was a problem hiding this comment.
I think you use this list only for a stub?
I think you should create abstraction like "graphs database" and use it instead of list.
| import java.util.Date; | ||
| import java.util.List; | ||
|
|
||
| public class GraphInitialize { |
| private String script = "KETER"; | ||
| private String parameters = "KETER"; | ||
| private List<List<String>> hardware = null; | ||
| private List<List<String>> files = null; |
There was a problem hiding this comment.
why list of list?
I think files should be list of string simply
There was a problem hiding this comment.
"filename_1" : "ceph://...",
"filename_2" : "redis://key",
...
"my_app.jar" : "http://..."
Ok. Really easier make list of string
| private String description = "KETER"; | ||
| private List<String> tags = null; | ||
| private String script = "KETER"; | ||
| private String parameters = "KETER"; |
There was a problem hiding this comment.
parameters is a Map<String, String> where key is a parameter name and value is a parameter value
| private List<String> tags = null; | ||
| private String script = "KETER"; | ||
| private String parameters = "KETER"; | ||
| private List<List<String>> hardware = null; |
There was a problem hiding this comment.
hardware is a Map<HardwareType, Long> where HardwareType - enum with type of hardware requirement and Long - value of needed resource
pom.xml
Outdated
| </properties> | ||
|
|
||
| <modules> | ||
| <module>keter-backend</module> |
There was a problem hiding this comment.
please fix "tabs" in your IDE
|
Also, } |
|
In other words user will send you all parameter, file paths and resource values and description of all dependencies in graph. |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Node { |
There was a problem hiding this comment.
where is input and output names of node template?
There was a problem hiding this comment.
And i think, m.b. send uuid from frontend? He is know him, bekause i send uuid for list of nodes.
Graph to GraphTemplate
Structure @RequestMapping
avplatonov
left a comment
avplatonov
left a comment
There was a problem hiding this comment.
Thank you for your contribution but please, be more attentive to my comments
|
|
||
| public static void main(String[] args) { | ||
| context = new AnnotationConfigApplicationContext(GraphsDB.class, NodesDB.class); | ||
| graphsDB = context.getBean(GraphsDB.class); |
There was a problem hiding this comment.
Use dep. injections through annontations
Don't use static fields and getBean explicitly
There was a problem hiding this comment.
| public CommandLineRunner commandLineRunner(ApplicationContext ctx) { | ||
| return args -> { | ||
|
|
||
| System.out.println("Let's inspect the beans provided by Spring Boot:"); |
There was a problem hiding this comment.
replace all printlns onto logging through slf4j
see my code in keter-core
| public String service( | ||
| @RequestBody GraphTemplate graphTemplate | ||
| ) throws IOException { | ||
| ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
As I know you can use only one ObjectMapper
why you create this object on every request?
| Set<NodeTemplate> listOfGraphsLocal = new HashSet<>(); | ||
| for (UUID graphUuid : uuidNodesToGraphTemplate.getListOfUuidNodes()) { | ||
| boolean containsNode = false; | ||
| for (NodeTemplate nodeTemplateUuid : nodesDB.getListOfNodeTemplates()) { |
There was a problem hiding this comment.
nodesDB.contains(nodeUUID) -> boolean
THIS is a abstraction over DB instead of manually iteration over all entities of DB
why you won't use HashMaps<UUID, NodeTemplate>?
| @RequestBody NodeTemplate nodeTemplate | ||
| ) throws IOException { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| nodesDB.addListOfNodes(nodeTemplate); |
There was a problem hiding this comment.
nodesDB.saveTemplate(...)
please use language of DB-level instead of collections-inspired language
|
|
||
| private Set<NodeTemplate> createGraph(GraphTemplate uuidNodesToGraphTemplate) { | ||
| Set<NodeTemplate> listOfGraphsLocal = new HashSet<>(); | ||
| for (UUID graphUuid : uuidNodesToGraphTemplate.getListOfUuidNodes()) { |
There was a problem hiding this comment.
iteration over all nodes
this is creepy
| } | ||
|
|
||
| private Set<NodeTemplate> createGraph(GraphTemplate uuidNodesToGraphTemplate) { | ||
| Set<NodeTemplate> listOfGraphsLocal = new HashSet<>(); |
There was a problem hiding this comment.
list of nodes, maybe?
copy-paste-sick?
| for (NodeTemplate nodeTemplateUuid : nodesDB.getListOfNodeTemplates()) { | ||
| if(graphUuid.equals(nodeTemplateUuid.getUuid())){ | ||
| containsNode = true; | ||
| listOfGraphsLocal.add(nodeTemplateUuid); |
There was a problem hiding this comment.
why graph is just set of node templates?
where is dependencies, where is parameters?
why graph is bag of shit in your interpretation?
| private List<String> searchGraph(String value) throws JsonProcessingException { | ||
| List<String> listOfGraphs = new ArrayList<>(); | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| for (NodeTemplate nodeTemplate : nodesDB.getListOfNodeTemplates()) { |
There was a problem hiding this comment.
iteration over DB
tell me if you want to check of containing of some element in SQL-DB you create "SELECT * FROM table" query and process "WHERE id=value"-part on java-side?
| private List<String> searchNode(String value) throws JsonProcessingException { | ||
| List<String> listOfNodes = new ArrayList<>(); | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| for (NodeTemplate nodeTemplate : nodesDB.getListOfNodeTemplates()) { |
2 participants
Release of management block