Improved API #19

Cerfoglg · 2016-05-25T17:25:45Z

Works on #16, works on #13, fixes #8

General Comment
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg does not build
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> You declared Make the implemented monitors clean and remove hard coded stuff #8 as fixed, but e.g., I don't see you add explicative comments. Check that Make the implemented monitors clean and remove hard coded stuff #8 is actually fixed.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme Fixed the deps (I hate golang so much), and added comments
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> You miss the .codeclimate file in the https://github.com/Cerfoglg/monitors/tree/env-variables branch related to this pull request.
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> In order to declare Improve the APIs exposed to the client #16 as fixed, you should document the APIs in the issue.
The same applies for Define the deployment descriptors and update ENV variables names #13
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg I reviewed the pull request. See all the comments remaining in the top comment of the issue.
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg you don' actually need to add the codeclimate file, just synch with the upstream repo because it is already there.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme There doesn't seem to be any codeclimate file on the upstream repo
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg https://github.com/benchflow/monitors/blob/dev/.codeclimate.yml
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg this is not done Improved API #19 (comment). It also impacts the description here: Improve the APIs exposed to the client #16 (comment)
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme This should all be okay now
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg reviewed again. You can see the remaining task in the list in the first comment.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme ready now
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L3-37
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Send also the actual result of the query back
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L40-70
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> What about the false alternative to the response?
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L40-70
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> You can put this (lines 56-63) in a function in commons, since I guess it is shared among all monitors.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme Depends on the response format. Better keep this for each monitor for now
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L180-221
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> This should be a REST API as well. It should manage the correct behaviour (200) and the erroneous behaviour as well.
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L180-221
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Why this is not returned to the caller, but just painted here?
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L180-221
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> If it is an ID, then you can exit the loop once you find the container passed as query
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L180-221
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Here you are setting the header every time you iterate the loop
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L40-70
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> You also need to set the status of the response. This applies in all the case where you send the response.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme That's an internalServerError status, so it's set
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Ok, but what about the other cases when you send the response back for the cpu and the mysql monitors?
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme It's set by default on 200 if you send data, even empty. Other parts it's specified.
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Fine
Vincenzo Ferme
Skype: vincenzoferme
Web: http://www.vincenzoferme.it
Email: info@vincenzoferme.it
On May 31, 2016 10:05 AM, "Gabriele Cerfoglio" notifications@github.com
wrote:
In dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go
Improved API #19 (comment):
```
            }
        }
 }
 db.Close()
```
- js, err := json.Marshal(response)
- if err != nil {
- http.Error(w, err.Error(), http.StatusInternalServerError)
- return
- }
@VincenzoFerme https://github.com/VincenzoFerme It's set by default on
200 if you send data, even empty. Other parts it's specified.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/benchflow/monitors/pull/19/files/1d99b54cec5c903516fc46c26c5dda469b79ffb4..88288c8c63362c49de6ee5089dc8ab418570c384#r65137974,
or mute the thread
https://github.com/notifications/unsubscribe/AG1O-cIOnuyiaAqff7vbMoc7gqbq0kgkks5qG-vegaJpZM4ImxyI
.
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L180-221
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> You should build the JSON representing the response, and then write it only once, after you json.Marshal it.
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L36-76
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Why do you need this variable?
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme turns out I don't, I had it in case I needed the row index. Removed it.
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L36-76
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Can't you do some assumption on the structure of the response, and directly pick the value for comparison? This way you are comparing line by line and you get it correct only if the actual value you want to compare is on the last line of the result.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme The assumption is already that the response should be a single row with a single value (like a count query), because managing completely arbitrary queries can be a mess to manage.
File: environment/cpu/src/cloud/benchflow/monitors/cpu.go:L246-253
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> each is not equal to Name. Set the actual value in a variable as you do for the ID, and then set it here.
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L48-70
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> else if
File: dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go:L48-70
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> else if

VincenzoFerme · 2016-05-25T19:23:10Z

@Cerfoglg does not build

VincenzoFerme · 2016-05-25T19:25:40Z

You declared #8 as fixed, but e.g., I don't see you add explicative comments. Check that #8 is actually fixed.

Cerfoglg · 2016-05-26T12:35:38Z

@VincenzoFerme Fixed the deps (I hate golang so much), and added comments

VincenzoFerme · 2016-05-26T17:52:10Z

dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go

 )

+type Response struct {


Send also the actual result of the query back

VincenzoFerme · 2016-05-26T17:53:55Z

You miss the .codeclimate file in the https://github.com/Cerfoglg/monitors/tree/env-variables branch related to this pull request.

VincenzoFerme · 2016-05-26T17:55:47Z

In order to declare #16 as fixed, you should document the APIs in the issue.

The same applies for #13

VincenzoFerme · 2016-05-26T17:58:34Z

dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go

@@ -39,19 +40,30 @@ func queryHandler(w http.ResponseWriter, r *http.Request) {
        }
        if name == value {


What about the false alternative to the response?

VincenzoFerme · 2016-05-26T18:20:46Z

@Cerfoglg I reviewed the pull request. See all the comments remaining in the top comment of the issue.

VincenzoFerme · 2016-05-27T11:38:03Z

dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go

@@ -48,14 +48,22 @@ func queryHandler(w http.ResponseWriter, r *http.Request) {
        		//fmt.Fprintf(w, "Row "+strconv.Itoa(rowI)+" matches "+value+" \n")
        		response = Response{true}
        		}
+        	if(method == "nequal") {


VincenzoFerme · 2016-05-27T11:38:40Z

@Cerfoglg you don' actually need to add the codeclimate file, just synch with the upstream repo because it is already there.

Cerfoglg · 2016-05-27T12:08:12Z

@VincenzoFerme There doesn't seem to be any codeclimate file on the upstream repo

VincenzoFerme · 2016-05-27T13:13:51Z

@Cerfoglg https://github.com/benchflow/monitors/blob/dev/.codeclimate.yml

VincenzoFerme · 2016-05-27T13:46:06Z

@Cerfoglg this is not done #19 (comment). It also impacts the description here: #16 (comment)

VincenzoFerme · 2016-05-27T13:54:43Z

dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go

 )

+// Struct for the Json respone for this monitor
+type Response struct {


Cerfoglg · 2016-05-30T11:13:29Z

@VincenzoFerme This should all be okay now

VincenzoFerme · 2016-05-30T20:17:27Z

dbms/sqlquery/mysql/src/cloud/benchflow/monitors/querymysql.go

@@ -35,35 +36,40 @@ func queryHandler(w http.ResponseWriter, r *http.Request) {
    }
    // Checks over the rows if the comparison holds
    rowI := 0


Why do you need this variable?

@VincenzoFerme turns out I don't, I had it in case I needed the row index. Removed it.

VincenzoFerme · 2016-05-30T20:29:26Z

@Cerfoglg reviewed again. You can see the remaining task in the list in the first comment.

Cerfoglg · 2016-06-02T08:20:31Z

@VincenzoFerme ready now

VincenzoFerme · 2016-06-02T10:33:42Z

👍

Cerfoglg added 3 commits May 5, 2016 17:36

altered env variable names

50e5362

Updated deps

1d99b54

Using JSON responses, adjustments in the code

88288c8

VincenzoFerme added the enhancement label May 25, 2016

VincenzoFerme added this to the Priorities milestone May 25, 2016

VincenzoFerme assigned Cerfoglg May 25, 2016

This was referenced May 25, 2016

Improve the APIs exposed to the client #16

Open

Define the deployment descriptors and update ENV variables names #13

Open

Cerfoglg added 2 commits May 26, 2016 13:18

Added missing deps

c14e01a

Added comments

bedc017

VincenzoFerme reviewed May 26, 2016
View reviewed changes

Cerfoglg added 2 commits May 27, 2016 13:28

Added codeclimate file

080b67c

Fixed alternative responses for mysqlquery

761b24d

VincenzoFerme reviewed May 27, 2016
View reviewed changes

Cerfoglg added 2 commits May 27, 2016 14:05

Corrections to responses

b3fb044

else if corrections

0c2bf4b

Cerfoglg added 2 commits May 27, 2016 15:38

Merge remote-tracking branch 'benchflow/dev' into env-variables

5866f97

Merged codeclimate

54db13f

VincenzoFerme reviewed May 27, 2016
View reviewed changes

Cerfoglg added 2 commits May 27, 2016 15:58

returning the query results

d9fe5e7

Can use container ID or container Name

3bf1f5a

VincenzoFerme reviewed May 30, 2016
View reviewed changes

Cerfoglg added 2 commits May 31, 2016 10:06

corrections

18d1776

Removed for loop

597511d

VincenzoFerme merged commit 6a7347a into benchflow:dev Jun 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved API #19

Improved API #19

Cerfoglg commented May 25, 2016 •

edited by VincenzoFerme

Loading

VincenzoFerme commented May 25, 2016

VincenzoFerme commented May 25, 2016

Cerfoglg commented May 26, 2016

VincenzoFerme May 26, 2016

VincenzoFerme commented May 26, 2016

VincenzoFerme commented May 26, 2016 •

edited

Loading

VincenzoFerme May 26, 2016

VincenzoFerme commented May 26, 2016

VincenzoFerme May 27, 2016

VincenzoFerme commented May 27, 2016

Cerfoglg commented May 27, 2016

VincenzoFerme commented May 27, 2016

VincenzoFerme commented May 27, 2016

VincenzoFerme May 27, 2016

Cerfoglg commented May 30, 2016

VincenzoFerme May 30, 2016

Cerfoglg May 31, 2016

VincenzoFerme commented May 30, 2016

Cerfoglg commented Jun 2, 2016

VincenzoFerme commented Jun 2, 2016

		@@ -39,19 +40,30 @@ func queryHandler(w http.ResponseWriter, r *http.Request) {
		}
		if name == value {

Improved API #19

Improved API #19

Conversation

Cerfoglg commented May 25, 2016 • edited by VincenzoFerme Loading

VincenzoFerme commented May 25, 2016

VincenzoFerme commented May 25, 2016

Cerfoglg commented May 26, 2016

VincenzoFerme May 26, 2016

Choose a reason for hiding this comment

VincenzoFerme commented May 26, 2016

VincenzoFerme commented May 26, 2016 • edited Loading

VincenzoFerme May 26, 2016

Choose a reason for hiding this comment

VincenzoFerme commented May 26, 2016

VincenzoFerme May 27, 2016

Choose a reason for hiding this comment

VincenzoFerme commented May 27, 2016

Cerfoglg commented May 27, 2016

VincenzoFerme commented May 27, 2016

VincenzoFerme commented May 27, 2016

VincenzoFerme May 27, 2016

Choose a reason for hiding this comment

Cerfoglg commented May 30, 2016

VincenzoFerme May 30, 2016

Choose a reason for hiding this comment

Cerfoglg May 31, 2016

Choose a reason for hiding this comment

VincenzoFerme commented May 30, 2016

Cerfoglg commented Jun 2, 2016

VincenzoFerme commented Jun 2, 2016

Cerfoglg commented May 25, 2016 •

edited by VincenzoFerme

Loading

VincenzoFerme commented May 26, 2016 •

edited

Loading