Fix aggregate output across the cluster #4815

li-ang · 2015-11-17T03:42:35Z

fix #4801
Ignoring time for aggregate output would lead to get wrong result when influxdb is working in cluster.
So, when the time of aggregate output isn't equal to zore, we shouldn't ignore time and return it in result.

CHANGELOG.md updated
Rebased/mergable
Tests pass
Sign CLA

otoolep · 2015-11-17T03:44:14Z

Thanks @li-ang -- we'll take a look.

otoolep · 2015-11-17T03:44:42Z

Have you signed the CLA @li-ang ?

li-ang · 2015-11-17T03:45:38Z

@otoolep Yes！

otoolep · 2015-11-17T03:46:19Z

OK, I see that -- thanks. CLA confirmed signed.

otoolep · 2015-11-17T03:48:07Z

cluster/shard_mapper.go

-			Tags:  mvj[0].Tags,
-		}}
+
+		if mvj[0].Time != 0 {


I'm not really sure why it this would make a difference. Either way you end up with a Time field equal to 0, right?

No, the mvj[0].Time maybe not equal to 0 when your aggregate query sentence has where time > xxx subsentence.

Still don't follow you. :-(

hi @otoolep, have you checked #4801 already? let's say if you execute select mean(value) from mem_used group by time(1m) , Time won't be zero because every interval has its own timestamp. however if you execute select mean(value) from mem_used without groupby, then Time will be ignored, and the value of Time is 0.

D'oh! I see it now, the old code was not setting the time field -- I never looked there. You are right. Thanks @li-ang

However, wouldn't this change work just as well:

mo.Values = []*tsdb.MapperValue{&tsdb.MapperValue{ + Time: mvj[0].Time, Value: aggValues, Tags: mvj[0].Tags, }}

In otherwords, just set the time.

Yes, that's right. just need add Time. @otoolep

and there will be another fix very soon following this PR.

li-ang · 2015-11-17T05:08:00Z

Done! @otoolep

CrazyJvm · 2015-11-17T05:48:37Z

cluster/shard_mapper.go

 			Value: aggValues,
 			Tags:  mvj[0].Tags,
 		}}
+


why blank line?

otoolep · 2015-11-17T19:29:21Z

This makes sense to me, thanks @li-ang +1

@dgnorton -- I need a review from you too.

corylanou · 2015-11-17T19:39:48Z

Not sure if it's easily done, but it would be nice to see a test that shows why this fixes the problem.

otoolep · 2015-11-17T19:42:22Z

Yeah, I agree @corylanou -- it's somewhat difficult in the remote mapping scenario, but I think it's possible.

otoolep · 2015-11-17T19:42:39Z

Setting the "force remote mapping" flag might do it -- that's what it's there for.

li-ang · 2015-11-18T03:34:12Z

The InfluxDB commit is d7ab25592360022fc10e0969e6e6f04cd22b665e

Results of setting the force-remote-mapping = false as following:

➜  influx git:(master) ./influx -host 192.168.1.62
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://192.168.1.62:8086 version 0.9
InfluxDB shell 0.9
> show servers
id  cluster_addr        raft    raft-leader
1   192.168.1.62:8088   true    true
2   192.168.1.64:8088   true    false
3   192.168.1.3:8088    true    false

> show shards
name: foo_62
------------
id  database    retention_policy    shard_group start_time  end_time        expiry_time     owners
1   foo_62      default         1       2015-11-16T00:00:00Z    2015-11-23T00:00:00Z    2015-11-23T00:00:00Z    1

> use foo_62
Using database foo_62

> show retention policies on foo_62
name    duration    replicaN    default
default 0       1       true

> select mean(float_value) from cpu where time > now() - 10m group by time(1m)
name: cpu
---------
time            mean
1447815780000000000 
1447815840000000000 
1447815900000000000 
1447815960000000000 
1447816020000000000 
1447816080000000000 
1447816140000000000 
1447816200000000000 0.4495545135199543
1447816260000000000 0.45045361601059564
1447816320000000000 0.45013218154087115
1447816380000000000 0.4511877603664354

>

➜  influx git:(master) ./influx -host 192.168.1.3 
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://192.168.1.3:8086 version 0.9
InfluxDB shell 0.9
> show servers
id  cluster_addr        raft    raft-leader
1   192.168.1.62:8088   true    true
2   192.168.1.64:8088   true    false
3   192.168.1.3:8088    true    false

> show shards
name: foo_62
------------
id  database    retention_policy    shard_group start_time  end_time        expiry_time     owners
1   foo_62      default         1       2015-11-16T00:00:00Z    2015-11-23T00:00:00Z    2015-11-23T00:00:00Z    1

> use foo_62
Using database foo_62
> show retention policies on foo_62
name    duration    replicaN    default
default 0       1       true

> select mean(float_value) from cpu where time > now() - 10m group by time(1m)
name: cpu
---------
time    mean
0   0.45079965450552983

>

Results of setting the force-remote-mapping = true as following:

> show shards
name: foo_62
------------
id  database    retention_policy    shard_group start_time  end_time        expiry_time     owners
1   foo_62      default         1       2015-11-16T00:00:00Z    2015-11-23T00:00:00Z    2015-11-23T00:00:00Z    1

> use foo_62
Using database foo_62
> show retention policies on foo_62
name    duration    replicaN    default
default 0       1       true

> select mean(float_value) from cpu where time > now() - 10m group by time(1m)
name: cpu
---------
time    mean
0   0.45028738264377094

> 

➜  influx git:(master) ./influx -host 192.168.1.3
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://192.168.1.3:8086 version 0.9
InfluxDB shell 0.9
> show servers
id  cluster_addr        raft    raft-leader
1   192.168.1.62:8088   true    false
2   192.168.1.64:8088   true    true
3   192.168.1.3:8088    true    false

> show shards
name: foo_62
------------
id  database    retention_policy    shard_group start_time  end_time        expiry_time     owners
1   foo_62      default         1       2015-11-16T00:00:00Z    2015-11-23T00:00:00Z    2015-11-23T00:00:00Z    1

> use foo_62
Using database foo_62
> select mean(float_value) from cpu where time > now() - 10m group by time(1m)
name: cpu
---------
time    mean
0   0.45069478050940354

>

li-ang · 2015-11-18T03:34:45Z

@otoolep @corylanou

otoolep · 2015-11-19T00:04:12Z

@dgnorton -- ping.

dgnorton · 2015-11-19T15:37:36Z

+1
I would say it needs tests but we don't have a good framework setup to add them now. @corylanou and I are working on that so this will be covered soon anyway.

Fix aggregate output across the cluster

otoolep · 2015-11-20T14:40:34Z

Thanks again @li-ang

otoolep reviewed Nov 17, 2015
View reviewed changes

li-ang changed the title ~~fix issue 4801~~ Fix aggregate output in cluster Nov 17, 2015

mengjinglei mentioned this pull request Nov 17, 2015

[0.9.5-nighly, cluster] some aggregate function work incorrectly in cluster mode #4816

Closed

CrazyJvm reviewed Nov 17, 2015
View reviewed changes

cluster/shard_mapper.go

Value: aggValues,

Tags: mvj[0].Tags,

}}

Copy link

Contributor

CrazyJvm Nov 17, 2015

Choose a reason for hiding this comment

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

why blank line?

liang@qiniu.com added 2 commits November 18, 2015 00:56

fix issue 4801

4026236

updated CHANGELOG.md

a712a1c

li-ang changed the title ~~Fix aggregate output in cluster~~ Fix aggregate output accross the cluster Nov 17, 2015

li-ang changed the title ~~Fix aggregate output accross the cluster~~ Fix aggregate output across the cluster Nov 17, 2015

otoolep added the review-pair label Nov 17, 2015

otoolep added a commit that referenced this pull request Nov 20, 2015

Merge pull request #4815 from li-ang/fix_4801

8ef5a05

Fix aggregate output across the cluster

otoolep merged commit 8ef5a05 into influxdata:master Nov 20, 2015

li-ang deleted the fix_4801 branch November 23, 2015 02:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix aggregate output across the cluster #4815

Fix aggregate output across the cluster #4815

li-ang commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep commented Nov 17, 2015

li-ang commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep Nov 17, 2015

li-ang Nov 17, 2015

otoolep Nov 17, 2015

CrazyJvm Nov 17, 2015

otoolep Nov 17, 2015

CrazyJvm Nov 17, 2015

li-ang commented Nov 17, 2015

CrazyJvm Nov 17, 2015

otoolep commented Nov 17, 2015

corylanou commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep commented Nov 17, 2015

li-ang commented Nov 18, 2015

li-ang commented Nov 18, 2015

otoolep commented Nov 19, 2015

dgnorton commented Nov 19, 2015

otoolep commented Nov 20, 2015

Fix aggregate output across the cluster #4815

Fix aggregate output across the cluster #4815

Conversation

li-ang commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep commented Nov 17, 2015

li-ang commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep Nov 17, 2015

Choose a reason for hiding this comment

li-ang Nov 17, 2015

Choose a reason for hiding this comment

otoolep Nov 17, 2015

Choose a reason for hiding this comment

CrazyJvm Nov 17, 2015

Choose a reason for hiding this comment

otoolep Nov 17, 2015

Choose a reason for hiding this comment

CrazyJvm Nov 17, 2015

Choose a reason for hiding this comment

li-ang commented Nov 17, 2015

CrazyJvm Nov 17, 2015

Choose a reason for hiding this comment

otoolep commented Nov 17, 2015

corylanou commented Nov 17, 2015

otoolep commented Nov 17, 2015

otoolep commented Nov 17, 2015

li-ang commented Nov 18, 2015

li-ang commented Nov 18, 2015

otoolep commented Nov 19, 2015

dgnorton commented Nov 19, 2015

otoolep commented Nov 20, 2015