Skip to content

Conversation

@blerer
Copy link
Contributor

@blerer blerer commented Mar 25, 2021

No description provided.

Copy link
Contributor

@ekaterinadimitrova2 ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

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

The tests are a bit lengthy to me and I had a few small format suggestions but otherwise it looks ok. Thank you.
Btw if I try to run in Intellij the whole class at once it easily OOMs if I don't raise the memory in advance

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there are 6 occurrences of "]" or ")", can we change it to ']' and ')' respectively

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

Comment on lines 61 to 63
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

Comment on lines 160 to 162
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

Comment on lines 89 to 102
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These are repetitive in both testMetricsWithStreamingToTwoNodes and testMetricsWithStreamingFromTwoNodes; maybe having some additional method and this way also reduce the length of the tests?

Comment on lines 215 to 216
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing spaces between cluster and 2

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space between cluster and 1

Comment on lines 220 to 221
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after the first comma

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would probably comment how we ended up comparing incomingProcessTime.getCount() to files, for clarity

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