search

go-graphite / go-whisper

A Go port of Graphite's Whisper timeseries database
BSD 3-Clause "New" or "Revised" License
16 stars 11 forks source link

cwhisper: fix int size build error on 32bit machines #10

Closed bom-d-van closed 4 years ago

bom-d-van commented 5 years ago

for isssue https://github.com/lomik/go-carbon/issues/320

Civil commented 5 years ago

Isn't it better to migrate to uint64 and not to rely on sizeof(int)?

bom-d-van commented 5 years ago

It might be a much bigger (breaking) change because we are accepting time as int (and there is a Y2038 issue in 32bits platform). Not sure if we want to invest more in supporting 32bits.

type TimeSeriesPoint struct {
    Time  int
    Value float64
}
bom-d-van commented 5 years ago

And it would still have a build error on 32 bits if we use uint64? With this simple fix, at least users could still get it up and running, but not recommend to use cwhisper on 32bits though (it might work, but it's built with 32bits support in mind).

Civil commented 5 years ago

It could be only internal conversion for cwhisper part.

I mean that the biggest issue here is that we are using type int and mostly expect it to be equal to int64, which is not true on some platforms.

So it seems to be logical to have internally int64 everywhere and do simple type conversion where needed.

Or maybe move Platform-dependent parts to a separate file with conditional compiling.

But anyway I'm ok with merging it as it is. Maybe it's just another issue to think about.