Skip to content

Conversation

@samperson1997
Copy link
Contributor

Create an interface for showing all the devices.

USAGE: SHOW DEVICES.

/**
* Get all devices in current Metadata Tree.
*
* @return a list contains all distinct storage groups
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return a list contains all distinct storage groups
* @return a list contains all distinct devices

* @return a list contains all distinct storage groups
*/
HashSet<String> getAllDevices() {
return new HashSet<>(getNodesList("3"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using an int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getNodesList method is implemented before and I just call it here, should I change the param type to int? (which I think is better as well)

}
case "SHOW_DEVICES":
Set<String> devices = getAllDevices();
resp.setShowDevices(devices);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resp.setShowDevices(devices);
resp.setDevices(devices);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I also keep consistent with SHOW TIMESERIES and SHOW STORAGE GROUP interface, in which they uses setShowTimeseriesList and setShowStorageGroups... Should I change all of these interfaces? (and I also think your suggestion is better...)

}
Assert.assertEquals(resultStr.toString(), standard);
} catch (SQLException e) {
System.out.println(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply print out the exception may bury the error and the TestCase will be regarded as PASSED. It's better to replace it with e.printStackTrace(); fail(e.getMessage());.

return mtree.getAllStorageGroup();
}

HashSet<String> getAllDevices() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that all its callers Interprete the returned value as Set but not HashSet.

@samperson1997
Copy link
Contributor Author

Hi @qiaojialin @kr11 ,

Thanks a lot for your detailed review! I have updated codes as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants