Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GRIFFIN-244 Get metrics by instance #491

Closed
wants to merge 27 commits into from
Closed

GRIFFIN-244 Get metrics by instance #491

wants to merge 27 commits into from

Conversation

dershovGD
Copy link
Contributor

No description provided.

dershovGD and others added 25 commits February 27, 2019 15:39
@dershovGD
Add applicationId to MetricWrapper
9351834 
In order to store more detailed information about certain metric the YARN application id associated with that metric has been added to MetricWrapper's flush method.
@dershovGD
Merge pull request #1 from apache/master
1c15379 
Merge new changes
@dershovGD
Merge pull request apache#2 from griddynamics/GRIFFIN-233-modify-Elas…
df8421a 
…ticSearchSink

Add applicationId to MetricWrapper
GRIFFIN-237 Implement service method getting JobInstanceBean by JobIn…
2b47428 
…stanceBean id
@dershovGD
Merge pull request apache#5 from griddynamics/Apply-Changes-231-234
4a1e31e 
Apply changes 231 234
uncomment mysql connector
13dac6a
@dershovGD
Handle exceptional situation.
62b6420 
Griffin Exception will be thrown if there is no Job Instance Bean with given id found. Test has been added to JobControllerTest to test this logic.
@dershovGD
Extend the documentation with new API method
e4ab309 
Add information about new method in documentation and create new postman example.
@aborgatin
Merge pull request apache#6 from griddynamics/feature/mysql_connector
6f22349 
uncomment mysql connector
@dershovGD
Merge pull request apache#7 from griddynamics/GRIFFIN-237
b3d712f 
Griffin 237 Get Job Instance by Id
Merge remote-tracking branch 'remotes/apache/master' into changes_fro…
259df93 
…m_upstream

# Conflicts:
#	service/src/main/java/org/apache/griffin/core/exception/GriffinExceptionMessage.java
#	service/src/main/java/org/apache/griffin/core/job/JobService.java
@aborgatin
Merge pull request apache#8 from griddynamics/feature/changes_feom_up…
5c432f5 
…stream

Changes from upstream
@dershovGD
Implement getting metrics by id method. Add test in MetricStoreImplTest.
3727e87
@dershovGD
Fix test MetricStoreImplTest
012da20
@dershovGD
Merge branch 'master' into Get-Metrics-By-Instance
1662f0e
@dershovGD
Uncomment mysql connector
a5e9bc8
@dershovGD
Adjust the request parameter
6e7c38e
@dershovGD
Reformat method findMetric of class MetricServiceImpl.Add tests to ve…
3c441e6 
…rify new logic.
@dershovGD
Merge remote-tracking branch 'originApache/master' into upstream_changes
1537157
@dershovGD
Merge branch 'master' into upstream_changes
741e48a 
# Conflicts:
#	service/src/main/java/org/apache/griffin/core/job/JobServiceImpl.java
#	service/src/test/java/org/apache/griffin/core/job/JobControllerTest.java
@dershovGD
Merge new features.
afe953e
@aborgatin
Merge pull request apache#10 from griddynamics/upstream_changes
44cc255 
Upstream changes
@dershovGD
Merge branch 'master' into Get-Metrics-By-Instance
c68579f
@dershovGD
Extend documentation
905557b
@dershovGD
Extend documentation
4308c85
@dershovGD dershovGD changed the title Get metrics by instance GRIFFIN-244 Get metrics by instance Mar 29, 2019
RodionGork
RodionGork suggested changes Mar 29, 2019
View reviewed changes
service/src/main/java/org/apache/griffin/core/exception/GriffinExceptionMessage.java
PREDICATE_TYPE_NOT_FOUND(40408, "Unknown predicate type"),
JOB_INSTANCE_NOT_FOUND(40408, "No job instances with given job instance id found"),

PREDICATE_TYPE_NOT_FOUND(40409, "Unknown predicate type"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably makes duplication with the next one. At least when requesting non-existing instance, I got Unknown predicate type in response :o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

service/pom.xml Outdated
@@ -110,11 +110,11 @@ under the License.
<artifactId>postgresql</artifactId>
<version>${postgresql.version}</version>
</dependency>
<!--<dependency>-->
Copy link
Contributor

Choose a reason for hiding this comment

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

let's revert it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've commented it back.

@dershovGD
Revert "Uncomment mysql connector"
cab2d74 
This reverts commit a5e9bc8
@dershovGD
Resolve collision with error codes.
0754d73 
After merging the different errors received the same status code(40409). This collision has been resolved.
RodionGork
RodionGork approved these changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@RodionGork RodionGork left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I approve. I wonder about whether we should check against empty array here but it looks like not real case for working application. Anyway this is not breaking any existing functionality.

Also there are some discrepancies with the project codestyle, but as existing project code also have them, we agreed to perform cleanup and attaching checkstyle in different PR.

@RodionGork
Copy link
Contributor

@chemikadze please come to see this...

RodionGork
RodionGork reviewed Apr 1, 2019
View reviewed changes
service/src/main/java/org/apache/griffin/core/metric/MetricStoreImpl.java
@Override
public MetricValue getMetric(String applicationId) throws IOException {
Map<String, String> map =new HashMap<>();
map.put("q", "applicationId:"+applicationId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Collections.singletonMap() probably can be used

@chemikadze
Copy link
Member

chemikadze commented Apr 2, 2019 •
edited
Loading

@RodionGork these merge commits have no practical sense, please rebase ontop of fresh master, and get rid of non-merge commits

@RodionGork
Copy link
Contributor

@chemikadze rebase leads to conflicts resolving... Thus I'd better have all this stuff squashed and pushed as @dershovGD (we probably have no practical sense in intermediate commits too), if no one's minds.

@RodionGork RodionGork mentioned this pull request Apr 2, 2019
Closed
@RodionGork
Copy link
Contributor

Now #492 is a new, squashed version of this PR.

@chemikadze chemikadze closed this Apr 3, 2019
asfgit pushed a commit that referenced this pull request Apr 14, 2019
@RodionGork @guoyuepeng
[GRIFFIN-244] get metrics by instance
ee2c7ac 
This is just a squashed version of #491

Author: Rodion Gorkovenko <rodiongork@github.com>
Author: rodion <dershov@griddynamics.com>

Closes #492 from RodionGork/griffin-244-squashed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RodionGork RodionGork RodionGork approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dershovGD @RodionGork @chemikadze @aborgatin