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.

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

Fix aggregate output across the cluster #4815

Merged
merged 2 commits into from
Nov 20, 2015
Merged

Fix aggregate output across the cluster #4815

merged 2 commits into from
Nov 20, 2015

Conversation

li-ang
Copy link

@li-ang li-ang commented Nov 17, 2015

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
Copy link
Contributor

otoolep commented Nov 17, 2015

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

@otoolep
Copy link
Contributor

otoolep commented Nov 17, 2015

Have you signed the CLA @li-ang ?

@li-ang
Copy link
Author

li-ang commented Nov 17, 2015

@otoolep Yes!

@otoolep
Copy link
Contributor

otoolep commented Nov 17, 2015

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

Tags: mvj[0].Tags,
}}

if mvj[0].Time != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Still don't follow you. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@li-ang li-ang changed the title fix issue 4801 Fix aggregate output in cluster Nov 17, 2015
@li-ang
Copy link
Author

li-ang commented Nov 17, 2015

Done! @otoolep

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

Copy link
Contributor

Choose a reason for hiding this comment

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

why blank line?

@li-ang li-ang changed the title Fix aggregate output in cluster Fix aggregate output accross the cluster Nov 17, 2015
@li-ang li-ang changed the title Fix aggregate output accross the cluster Fix aggregate output across the cluster Nov 17, 2015
@otoolep
Copy link
Contributor

otoolep commented Nov 17, 2015

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

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

@corylanou
Copy link
Contributor

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

@otoolep
Copy link
Contributor

otoolep commented Nov 17, 2015

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

@otoolep
Copy link
Contributor

otoolep commented Nov 17, 2015

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

@li-ang
Copy link
Author

li-ang commented Nov 18, 2015

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
Copy link
Author

li-ang commented Nov 18, 2015

@otoolep @corylanou

@otoolep
Copy link
Contributor

otoolep commented Nov 19, 2015

@dgnorton -- ping.

@dgnorton
Copy link
Contributor

+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.

otoolep added a commit that referenced this pull request Nov 20, 2015
Fix aggregate output across the cluster
@otoolep otoolep merged commit 8ef5a05 into influxdata:master Nov 20, 2015
@otoolep
Copy link
Contributor

otoolep commented Nov 20, 2015

Thanks again @li-ang

@li-ang li-ang deleted the fix_4801 branch November 23, 2015 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.9.5-rc2: Cluster : get wrong result when add new node and query through this node with aggregation function.
5 participants