Review #2
Review #2
Conversation
src/main/java/org/iot/dsa/dslink/dynamodb/DynamoDBDSAClient.java
Outdated
Show resolved
Hide resolved
| ListTablesResult tables = client.listTables(); | ||
| List tnames = tables.getTableNames(); | ||
| for(int i = 0 ; i < tnames.size();i++){ | ||
| System.out.println(tnames.get(i)); |
There was a problem hiding this comment.
System.out.println shouldn't be used. If necessary to write to the log, extend DSLogger and call one of the logging methods.
There was a problem hiding this comment.
There are still some System.out.println calls in Util.java, could you change those as well?
src/main/java/org/iot/dsa/dslink/dynamodb/DynamoDBDSAClient.java
Outdated
Show resolved
Hide resolved
| return setN; | ||
| } | ||
|
|
||
| public static Map parseHashMap(DSMap m){ |
There was a problem hiding this comment.
Map, List, Set, etc. should be parameterized (e.g. Map<String, String>)
There was a problem hiding this comment.
I'm still seeing unparameterized references to ArrayList, List, and Set in Util.java, please change those as well.
src/main/java/org/iot/dsa/dslink/dynamodb/DynamoDBDSAClient.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| if (endpoint.getElement().toString().isEmpty()) { | ||
| configFault("Empty End-Point"); |
There was a problem hiding this comment.
Shouldn't it be okay to have an empty endpoint? (Since you handle that case on line 55 of Util.java)
There was a problem hiding this comment.
In Util we included because we were testing for local DynamoDB. But for clound connection this is mandatory field.
There was a problem hiding this comment.
I don't think this is always the case. I commented out that check and was able to connect without specifying an endpoint, region was enough.
There was a problem hiding this comment.
We tested with local dynamodb. commenting out and without specifying endurl we are getting "java.lang.IllegalStateException: The security token included in the request is invalid. (Service: AmazonDynamoDBv2; Status Code: 400; Error Code: UnrecognizedClientException; Request ID: GRR8OEUBBU2EMMH8L4L3MU4A7JVV4KQNSO5AEMVJF66Q9ASUAAJG)
".
So not sure how it behaves with cloud dynamodb. So is it possible if you have test accesskey and secretkey to share us? So that we test at our end. But we feel there is no harm to keep this code snippet.
| } | ||
| }; | ||
| act.addParameter(Constants.TABLENAME, DSValueType.STRING, "Table Name"); | ||
| act.addParameter(Constants.PROJECTIONEXPRESSION, DSValueType.STRING, "ProjectionExpression"); |
There was a problem hiding this comment.
We really need some documentation on how to use all these actions (Query, Scan, Put Item, etc.)
There was a problem hiding this comment.
There was a problem hiding this comment.
Great! Could you set that as the help url? Currently it just reads "TODO : Need to upload in github"
No description provided.