Skip to content

Adding two additional params to DiagnosticsStreamMessage - #1126

Merged
rmatharu-zz merged 3 commits into
apache:masterfrom
rmatharu-zz:test-add-diagnostics-stream-message
Aug 7, 2019
Merged

rmatharu-zz merged 3 commits into
apache:masterfrom
rmatharu-zz:test-add-diagnostics-stream-message

Conversation

@rmatharu-zz

Copy link
Copy Markdown
Contributor

No description provided.

@mynameborat mynameborat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall. Minor comment and also please attach the jira to the PR.

public DiagnosticsManager(String jobName, String jobId, Map<String, ContainerModel> containerModels,
Integer containerMemoryMb, Integer containerNumCores, Integer numStoresWithChangelog, String containerId,
String executionEnvContainerId, String taskClassVersion, String samzaVersion, String hostname,
Integer containerMemoryMb, Integer containerNumCores, Integer numStoresWithChangelog, Long maxHeapSizeBytes, Integer containerThreadPoolSize,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we add the new parameters at the end?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we could, i wanted to try and keep the sizing-related params together, container-id related together, and the remaining at the end.

@cameronlee314 cameronlee314 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a few minor things from me too

Comment thread samza-core/src/main/java/org/apache/samza/util/DiagnosticsUtil.java Outdated
Comment thread samza-core/src/main/scala/org/apache/samza/diagnostics/DiagnosticsManager.java Outdated
Comment thread samza-core/src/main/scala/org/apache/samza/diagnostics/DiagnosticsManager.java Outdated
Comment thread samza-core/src/main/scala/org/apache/samza/diagnostics/DiagnosticsManager.java Outdated
@rmatharu-zz

Copy link
Copy Markdown
Contributor Author

Addressed all comments.

Comment thread samza-core/src/main/scala/org/apache/samza/diagnostics/DiagnosticsManager.java Outdated
@rmatharu-zz
rmatharu-zz merged commit f3d9494 into apache:master Aug 7, 2019
@cameronlee314

Copy link
Copy Markdown
Contributor

@rmatharu Please remember to include the JIRA ticket number in the commit message.

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.

4 participants