NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion #1460

tzzed · 2021-09-10T21:38:16Z

What does this do?

This PR is a simple refactoring of influxdb measurement data.

Which issue(s) does this PR fix/relate to?

No issue related to this PR.

List any changes that modify/break current functionality

Have you included tests for your changes?

Still working

Did you document any new/modified functionality?

No

Notes

Signed-off-by: tzzed <zerouali.t@gmail.com>

readers/influxdb/messages.go

readers/influxdb/messages_test.go

drasko · 2021-09-10T22:21:35Z

@tzzed thanks for the PR. Please sync your branch to the master.

@arvindh123 please review this PR

…factore/reader_influxdb

codecov-commenter · 2021-09-11T12:18:48Z

Codecov Report

Merging #1460 (5767423) into master (b570c38) will decrease coverage by 0.02%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
- Coverage   68.80%   68.78%   -0.03%     
==========================================
  Files         132      132              
  Lines       10581    10587       +6     
==========================================
+ Hits         7280     7282       +2     
- Misses       2726     2727       +1     
- Partials      575      578       +3

Impacted Files	Coverage Δ
readers/influxdb/messages.go	`78.44% <61.11%> (-2.93%)`	⬇️
things/service.go	`74.45% <0.00%> (+0.86%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b570c38...5767423. Read the comment docs.

Signed-off-by: tzzed <zerouali.t@gmail.com>

readers/influxdb/messages.go

arvindh123

@drasko, It looks good

drasko

Honestly, I do not see value in this PR. I would like that @dusanb94 reviews, but I do not find new code better or more comprehensible.

drasko · 2021-09-13T18:27:37Z

readers/influxdb/messages_test.go

 		for i := 0; i < len(result.Messages); i++ {
 			m := result.Messages[i]
 			// Remove time as it is not sent by the client.
 			delete(m.(map[string]interface{}), "time")

 			result.Messages[i] = m
 		}
-		assert.Nil(t, err, fmt.Sprintf("%s: expected no error got %s", desc, err))


Why is this assert removed?

The assert checks the error after the loop but it should be done before just after getting the result, err := reader.ReadAll(tc.chanID, tc.pageMeta) .

Is it done?

Again - is this done?

I put it after reader.ReadAll(tc.chanID, tc.pageMeta)

This looks ok but in any case don't resolve remarks by hand please.

tzzed · 2021-09-15T22:17:35Z

the len's comparison to zero is more clear because we want to check the 0 value an empty array. len(v) < 1 includes negatives values.

About the safe cast is just to avoid panic.

I just let myself to do this PR as a "good first issue" but you are free to close it.

Signed-off-by: tzzed <zerouali.t@gmail.com>

drasko · 2021-09-16T23:33:18Z

@nmarcetic and @dusanb94 need to review and approve this one.

readers/influxdb/messages.go

drasko · 2021-09-16T23:34:13Z

readers/influxdb/messages.go

 		return readers.MessagesPage{}, nil
 	}

+	var messages []readers.Message
+


No need for blank line

drasko · 2021-09-16T23:34:33Z

readers/influxdb/messages.go

@@ -140,7 +140,8 @@ func fmtCondition(chanID string, rpm readers.PageMetadata) string {
 	if err != nil {
 		return condition
 	}
-	json.Unmarshal(meta, &query)
+


No need for blank line.

drasko · 2021-09-16T23:35:30Z

readers/influxdb/messages.go

@@ -140,7 +140,8 @@ func fmtCondition(chanID string, rpm readers.PageMetadata) string {
 	if err != nil {
 		return condition
 	}
-	json.Unmarshal(meta, &query)
+
+	_ = json.Unmarshal(meta, &query)


Do not ignore errors.

readers/influxdb/messages.go

readers/influxdb/messages_test.go

drasko · 2021-09-16T23:37:18Z

readers/influxdb/messages_test.go

 		for i := 0; i < len(result.Messages); i++ {
 			m := result.Messages[i]
 			// Remove time as it is not sent by the client.
 			delete(m.(map[string]interface{}), "time")

 			result.Messages[i] = m
 		}
-		assert.Nil(t, err, fmt.Sprintf("%s: expected no error got %s", desc, err))


Is it done?

Signed-off-by: tzzed <zerouali.t@gmail.com>

dborovcanin · 2021-10-11T11:05:05Z

Honestly, I do not see value in this PR. I would like that @dusanb94 reviews, but I do not find new code better or more comprehensible.

The refactoring makes sense. @tzzed please consider adding better error handling and InfluxDB records parsing. Those improvements can be added to this PR to improve its value.

readers/influxdb/messages.go

drasko · 2021-10-11T16:27:50Z

readers/influxdb/messages_test.go

 		for i := 0; i < len(result.Messages); i++ {
 			m := result.Messages[i]
 			// Remove time as it is not sent by the client.
 			delete(m.(map[string]interface{}), "time")

 			result.Messages[i] = m
 		}
-		assert.Nil(t, err, fmt.Sprintf("%s: expected no error got %s", desc, err))


Again - is this done?

drasko · 2021-10-11T16:29:05Z

Honestly, I do not see value in this PR. I would like that @dusanb94 reviews, but I do not find new code better or more comprehensible.

The refactoring makes sense. @tzzed please consider adding better error handling and InfluxDB records parsing. Those improvements can be added to this PR to improve its value.

OK, then I left a few comments, and these must be corrected before we are ready to merge.

drasko · 2021-10-15T22:24:45Z

@tzzed if there is no action from your side to correct mentioned comment, this PR will be closed due to the inactivity.

manuio · 2021-10-21T14:11:40Z

readers/influxdb/messages_test.go

@@ -6,6 +6,7 @@ import (
 	"time"

 	influxdata "github.com/influxdata/influxdb/client/v2"
+


Remove this blank line

Signed-off-by: tzzed <zerouali.t@gmail.com>

drasko · 2021-10-21T18:37:35Z

@tzzed CI is failing. Please check your tests, you have introduced regression:

    messages_test.go:378: 
        	Error Trace:	messages_test.go:378
        	Error:      	Not equal: 
        	            	expected: 0x4f
        	            	actual  : 0x64
        	Test:       	TestReadAll
        	Messages:   	read message with to: expected 79 got 100
    messages_test.go:378: 
        	Error Trace:	messages_test.go:378
        	Error:      	Not equal: 
        	            	expected: 0x15
        	            	actual  : 0x64
        	Test:       	TestReadAll
        	Messages:   	read message with from: expected 21 got 100
--- FAIL: TestReadAll (0.62s)
=== RUN   TestReadJSON
--- PASS: TestReadJSON (0.12s)
FAIL
coverage: 86.4% of statements
FAIL	github.com/mainflux/mainflux/readers/influxdb	20.089s
FAIL

tzzed · 2021-10-21T22:28:50Z

@drasko I did not introduced regression. This is because of the msg.Time setting. now is in second but when it is equal to 0, 5, or 20 it fails. I invite you to try to set it into this 3 values in the master branch and run the test.

now := float64(time.Now().UTC().Second())

	for i := 0; i < msgsNum; i++ {
		// Mix possible values as well as value sum.
		msg := m
		msg.Time = now - float64(i)  // This operations has a random fails or success.

I will change it tomorrow if you are ok @drasko ? It will add a real value to this PR.

Signed-off-by: tzzed <zerouali.t@gmail.com>

dborovcanin · 2021-10-24T20:15:11Z

readers/influxdb/messages_test.go

-	now := float64(time.Now().UTC().Second())
+	rand.Seed(time.Now().UnixNano())
+	to := msgsNum
+	from := 21


You can use this as a constant in tests with a range search instead of literals, as well.

@tzzed I also meant to use this constant later in the code: https://github.com/mainflux/mainflux/blob/master/readers/influxdb/messages_test.go#L334-L355

I have seen this line but I have not enough context about this lines. How do you see the usage.Should I create a const for each value in test.

pageMeta: readers.PageMetadata{ Offset: 0, Limit: uint64(len(messages[21:])), To: messages[20].Time, },

No need, I think. Replacing literals 20 and 21 with from and from + 1 (and maybe rename from to offset) will be just fine.

Signed-off-by: tzzed <zerouali.t@gmail.com>

drasko

LGTM

drasko

LGTM

…onversion (absmach#1460) * refactore(reader/influxdb): explicit check event + add safe conversion Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion Signed-off-by: tzzed <zerouali.t@gmail.com> * Delete useless space. Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion, replace require.NoError(t, err) by assert.Nil Signed-off-by: tzzed <zerouali.t@gmail.com> * Fix tests when message time is randomly equal to 0 in tests cases. Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion use constant Signed-off-by: tzzed <zerouali.t@gmail.com> * use const offset Signed-off-by: tzzed <zerouali.t@gmail.com> Co-authored-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

…onversion (absmach#1460) * refactore(reader/influxdb): explicit check event + add safe conversion Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion Signed-off-by: tzzed <zerouali.t@gmail.com> * Delete useless space. Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion, replace require.NoError(t, err) by assert.Nil Signed-off-by: tzzed <zerouali.t@gmail.com> * Fix tests when message time is randomly equal to 0 in tests cases. Signed-off-by: tzzed <zerouali.t@gmail.com> * apply review suggestion use constant Signed-off-by: tzzed <zerouali.t@gmail.com> * use const offset Signed-off-by: tzzed <zerouali.t@gmail.com> Co-authored-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> Signed-off-by: skovacevic <stefan.kovacevic@mainflux.io>

tzzed requested a review from a team as a code owner September 10, 2021 21:38

refactore(reader/influxdb): explicit check event + add safe conversion

f9e5eac

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from 7da2b28 to f9e5eac Compare September 10, 2021 21:39

drasko requested changes Sep 10, 2021

View reviewed changes

readers/influxdb/messages.go Outdated Show resolved Hide resolved

readers/influxdb/messages_test.go Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/mainflux/mainflux into re…

7a28715

…factore/reader_influxdb

tzzed force-pushed the refactore/reader_influxdb branch 2 times, most recently from b2b2916 to 04e569e Compare September 11, 2021 12:16

tzzed force-pushed the refactore/reader_influxdb branch 2 times, most recently from 0efd3fc to 0508757 Compare September 11, 2021 12:23

apply review suggestion

7d1b9c2

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from 0508757 to 7d1b9c2 Compare September 11, 2021 12:32

tzzed requested a review from drasko September 13, 2021 08:38

manuio suggested changes Sep 13, 2021

View reviewed changes

readers/influxdb/messages.go Outdated Show resolved Hide resolved

arvindh123 reviewed Sep 13, 2021

View reviewed changes

drasko changed the title ~~NOISSUE - refactore(reader/influxdb): explicit check event + add safe conversion~~ NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion Sep 13, 2021

drasko requested changes Sep 13, 2021

View reviewed changes

Delete useless space.

843c7f4

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from fc4208b to 843c7f4 Compare September 15, 2021 22:28

tzzed requested a review from drasko September 16, 2021 22:34

drasko requested review from dborovcanin and nmarcetic September 16, 2021 23:32

drasko requested changes Sep 16, 2021

View reviewed changes

apply review suggestion

fdcb7d7

Signed-off-by: tzzed <zerouali.t@gmail.com>

Merge branch 'master' into refactore/reader_influxdb

f0ab071

drasko requested changes Oct 11, 2021

View reviewed changes

Merge branch 'master' into refactore/reader_influxdb

62f63b4

tzzed force-pushed the refactore/reader_influxdb branch from 3625173 to 10783dd Compare October 21, 2021 12:20

tzzed requested a review from drasko October 21, 2021 12:21

manuio reviewed Oct 21, 2021

View reviewed changes

apply review suggestion, replace require.NoError(t, err) by assert.Nil

c00ee1c

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from 10783dd to c00ee1c Compare October 21, 2021 17:54

Fix tests when message time is randomly equal to 0 in tests cases.

d2bba44

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from 63c4bfb to d2bba44 Compare October 22, 2021 22:11

dborovcanin reviewed Oct 24, 2021

View reviewed changes

apply review suggestion use constant

69927e7

Signed-off-by: tzzed <zerouali.t@gmail.com>

tzzed force-pushed the refactore/reader_influxdb branch from 5379f78 to 69927e7 Compare October 25, 2021 11:49

tzzed requested review from dborovcanin and arvindh123 October 25, 2021 15:11

use const offset

233c616

Signed-off-by: tzzed <zerouali.t@gmail.com>

drasko approved these changes Oct 26, 2021

View reviewed changes

drasko approved these changes Nov 8, 2021

View reviewed changes

Merge branch 'master' into refactore/reader_influxdb

5767423

dborovcanin approved these changes Nov 8, 2021

View reviewed changes

blokovi approved these changes Nov 8, 2021

View reviewed changes

drasko merged commit 9f5a319 into absmach:master Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion #1460

NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion #1460

tzzed commented Sep 10, 2021

drasko commented Sep 10, 2021

codecov-commenter commented Sep 11, 2021 •

edited

Loading

arvindh123 left a comment

drasko left a comment

drasko Sep 13, 2021

tzzed Sep 15, 2021

drasko Sep 16, 2021

drasko Oct 11, 2021

tzzed Oct 21, 2021 •

edited

Loading

manuio Oct 21, 2021

tzzed commented Sep 15, 2021

drasko commented Sep 16, 2021

drasko Sep 16, 2021

drasko Sep 16, 2021

drasko Sep 16, 2021

drasko Sep 16, 2021

dborovcanin commented Oct 11, 2021

drasko Oct 11, 2021

drasko commented Oct 11, 2021

drasko commented Oct 15, 2021

manuio Oct 21, 2021

tzzed Oct 21, 2021

drasko commented Oct 21, 2021

tzzed commented Oct 21, 2021

dborovcanin Oct 24, 2021

dborovcanin Oct 25, 2021

tzzed Oct 25, 2021

dborovcanin Oct 25, 2021

drasko left a comment

drasko left a comment

		@@ -6,6 +6,7 @@ import (
		"time"

		influxdata "github.com/influxdata/influxdb/client/v2"

NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion #1460

NOISSUE - Refactor InfluxDB Reader: explicit check event + add safe conversion #1460

Conversation

tzzed commented Sep 10, 2021

What does this do?

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

drasko commented Sep 10, 2021

codecov-commenter commented Sep 11, 2021 • edited Loading

Codecov Report

arvindh123 left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzzed Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzzed commented Sep 15, 2021

drasko commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dborovcanin commented Oct 11, 2021

Choose a reason for hiding this comment

drasko commented Oct 11, 2021

drasko commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko commented Oct 21, 2021

tzzed commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 11, 2021 •

edited

Loading

tzzed Oct 21, 2021 •

edited

Loading