-
Notifications
You must be signed in to change notification settings - Fork 80
IGNITE-13860 Provide the tool to build cluster performance report #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observed we have an open PR in ignite repo for this feature with different set of changes.
Can you please share more info on how we can use the profiling tool with ignite-extensions modules?
modules/profiling-ext/README.txt
Outdated
| @@ -0,0 +1,48 @@ | |||
| Apache Ignite Profiling Module | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project's readme-files does not contain a license header. Why we should add it?
| http://maven.apache.org/xsd/assembly-1.1.2.xsd"> | ||
| <id>profiling-report-sources</id> | ||
|
|
||
| <includeBaseDirectory>false</includeBaseDirectory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please share details on this base dir flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have excluded base directory from UI sources archive:
report.zip/{sources}
instead of
report.zip/report/{sources}
| * Version: 2.7.3 | ||
| * | ||
| * Copyright 2018 Chart.js Contributors | ||
| * Released under the MIT license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ok to use mix of MIT and Apache license?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok, I have mentioned these components in the LICENSE file.
| @@ -0,0 +1,2 @@ | |||
| /*! jQuery v3.3.1 | (c) JS Foundation and other contributors | jquery.org/license */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to add license header in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have clarified the license.
| Query query = new Query(type, text, queryNodeId, id, startTime, duration, success); | ||
|
|
||
| OrderedFixedSizeStructure<Long, Query> tree = topSlow.computeIfAbsent(type, | ||
| t -> new OrderedFixedSizeStructure<>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename t to a variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| long physicalReads) { | ||
|
|
||
| Map<Long, long[]> ids = readsById.computeIfAbsent(type, t -> new HashMap<>()) | ||
| .computeIfAbsent(queryNodeId, k -> new HashMap<>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename k to a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed and shared comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
|
@NSAmelchev There is conflicts in this PR, can you please resolve the conflict and then go ahead and merge the changes. |
| # | ||
| # Run tool. | ||
| # | ||
| Java ${JVM_OPTS} -cp "${CP}" ${MAIN_CLASS} $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Java -> java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| <resource> | ||
| <directory>${basedir}/target</directory> | ||
| <includes> | ||
| <include>report.zip</include> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include UI resources as a directory not zip archive?
|
@NSAmelchev Can we modify build process so it will download css, js resources in the build time? |
Cluster profiling tool based on logging performance statistics to files. See apache/ignite#7693
Run command to build the tool:
mvn clean installRelease archive will be be under the directory:
modules/performance-statistics-ext/target/release-package-ignite-performance-statistics.zipTo collect statistics in runtime and to build the performance report follow:
PerformanceStatisticsMBeanPerformanceStatisticsMBeanbuild-report.sh path_to_filesto build the performance report.The report contains statistics:
get|getAll|put|putAll|remove|removeAll|getAndPut|getAndRemove|lock|invoke|invokeAll