Skip to content

Conversation

@Al2Klimov
Copy link
Member

No description provided.

@Al2Klimov Al2Klimov added the needs-feedback We'll only proceed once we hear from you again label Feb 4, 2021
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Just some first comments for now.

@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch from b1a9720 to 70d901a Compare April 6, 2021 11:39
@Al2Klimov
Copy link
Member Author

Now it should work as expected. Please have a look.

@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch from 70d901a to 3b3f6d2 Compare April 23, 2021 09:55
@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch 3 times, most recently from def926e to 3eb22d9 Compare August 3, 2021 13:50
@lippserd lippserd added this to the v1.0.0-rc2 milestone Aug 12, 2021
@julianbrost julianbrost mentioned this pull request Sep 7, 2021
@Al2Klimov
Copy link
Member Author

1 rewrite, 42 quadrillion centuries and one s/Debian/Ubuntu/ later...

@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch from 3eb22d9 to a2b2286 Compare September 30, 2021 13:32
@cla-bot cla-bot bot added the cla/signed label Sep 30, 2021
@Al2Klimov
Copy link
Member Author

Al2Klimov commented Sep 30, 2021

TODO

  • rebase
  • conflicts
  • benchmarks
  • docs
  • envs in deterministic UUIDs?

@Al2Klimov Al2Klimov requested a review from N-o-X September 30, 2021 13:35
@Al2Klimov Al2Klimov changed the title tools/ido2icingadb: migrate history from IDO to Icinga DB cmd/ido2icingadb: migrate history from IDO to Icinga DB Sep 30, 2021
@Al2Klimov
Copy link
Member Author

Setup

  • 12 CPU
  • 32 GB RAM
  • 75 GB IDO DB
  • all on one host

Full migration

  • took 1269m19.229s
  • no interrupts

@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch 6 times, most recently from cd8977e to 7936077 Compare October 1, 2021 10:41
@julianbrost
Copy link
Contributor

Do I understand correctly that the time range limitation also applies to populating the cache, i.e. it won't take earlier rows into account, even if they would hold information (like the previous state) for a later event within the migration time range?

Not sure yet if that's good or bad. If you restrict the lower limit, you probably have a reason. If we want to fill as much as possible, we'd have to build the cache from the very beginning, so it might scan 5 years to migrate 1 year. That might not be desired, on the other hand, reading doesn't seem to be the bottleneck.

@Al2Klimov
Copy link
Member Author

Oh. You're right. 😅

Comment on lines 22 to 97
// slowlyIncrementingProgressDecorator is the base decorator
// for possibly slowly incrementing progresses possibly starting from >0%.
type slowlyIncrementingProgressDecorator struct {
decor.WC

// startProgress is the first progress >0 seen by Decor.
startProgress int64
// startTime tells when is startProgress from.
startTime time.Time
// lastProgress is the last progress >0 seen by Decor.
lastProgress int64
// lastTime tells when is lastProgress from.
lastTime time.Time
}

// update is to be called by Decor and returns whether sipd is warmed up.
func (sipd *slowlyIncrementingProgressDecorator) update(s decor.Statistics) bool {
if s.Completed || s.Current < 1 {
return false
}

if sipd.startProgress < 1 {
sipd.startProgress = s.Current
sipd.startTime = time.Now()
sipd.lastProgress = sipd.startProgress
sipd.lastTime = sipd.startTime

return false
}

if s.Current == sipd.startProgress {
return false
}

if s.Current > sipd.lastProgress {
sipd.lastProgress = s.Current
sipd.lastTime = time.Now()
}

return true
}

// eta indicates the ETA for possibly slowly incrementing progresses possibly starting from >0%.
type eta struct {
slowlyIncrementingProgressDecorator
}

// Decor implements the decor.Decorator interface.
func (e *eta) Decor(s decor.Statistics) string {
if e.update(s) {
timePerItem := float64(e.lastTime.Sub(e.startTime)) / float64(e.lastProgress-e.startProgress)
lastETA := time.Duration(float64(s.Total-s.Current) * timePerItem)

return e.FormatMsg(((lastETA - time.Since(e.lastTime)) / time.Second * time.Second).String())
} else {
return ""
}
}

// eta indicates ops/s for possibly slowly incrementing progresses possibly starting from >0%.
type opsPerSec struct {
slowlyIncrementingProgressDecorator
}

// Decor implements the decor.Decorator interface.
func (ops *opsPerSec) Decor(s decor.Statistics) string {
if ops.update(s) {
return ops.FormatMsg(fmt.Sprintf(
"%.0f/s",
float64(ops.lastProgress-ops.startProgress)/
(float64(ops.lastTime.Sub(ops.startTime))/float64(time.Second)),
))
} else {
return ""
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The progress bars use the average from start to show rates and time estimates. Given that there are index updates involved when inserting, insert complexity should be (at least) logarithmic, i.e. how the insert rate progresses over time can be approximated by 1/ln(x). So inserting will likely only get slower over time (unless there's a magic performance improvement on the database server in the meantime), making the values displayed too optimistic.

Thus, I think displaying an average value over the last few minutes or transactions (to avoid having the exact numbers jump around like crazy) would produce more useful numbers here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it worth investing 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.

Shouldn't take that much time I think. Should just need a slice of progress/time pairs where you keep the last like 10 updates and then perform the same calculation just with the oldest and newest values from that slice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to continue reviewing while I'm on it.

@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch from d099855 to 649f7e0 Compare October 24, 2022 14:31
@Al2Klimov Al2Klimov requested a review from julianbrost October 24, 2022 14:46
@Al2Klimov Al2Klimov added this to the 1.1.0 milestone Oct 25, 2022
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Apart from the inline comment, I think you can start doing the other things that will be needed to get this ready:

  1. Rename the cmd/ido2icingadb/ to the final name (probably icingadb-migrate as it already says in the README).
  2. Integrate the README into doc/.

@Al2Klimov Al2Klimov requested a review from julianbrost October 25, 2022 16:25
@Al2Klimov Al2Klimov force-pushed the feature/migrate-history branch from d1f1dc1 to ec4750d Compare October 26, 2022 08:25
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Please have a look at the paths, that rename went wrong.

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Code should be done now, just getting the docs ready left I think. In general, I miss a clear "do this because this will probably be what you want" with some "in some cases you might want this instead" options.

Comment on lines +37 to +48
type: pgsql
host: 192.0.2.1
port: 5432
database: icinga
user: icinga
password: CHANGEME
# Input time range
#from: 0
#to: 2147483647
# Icinga DB database
icingadb:
type: mysql
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd not mix database types here, might lead to confusion. I get that this is supposed to say that both types are supported, but I think that would be easier to understand if both occurrences said type: mysql # or pgsql for PostgreSQL.

Copy link
Member Author

Choose a reason for hiding this comment

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

This shall also express that cross-RDBMS-type migration is supported.

@Al2Klimov
Copy link
Member Author

?

@julianbrost
Copy link
Contributor

?

?

(If you're wondering about an incomplete sentence you've read in a notification e-mail, please read it again on the website.)

(PS: I hate ISO keyboard layouts where the backspace and return key are directly next to each other.)

@Al2Klimov
Copy link
Member Author

Er, no, I've read #253 (review) on the website.

@julianbrost
Copy link
Contributor

I think for almost all users, the workflow of setting up Icinga DB in parallel and then migrating everything up to that point in time where they started running Icinga DB is what they want. With the current state of the documentation, they have to figure this out themselves but I think this should be documented as the way to go. This can then have extra sections for things like "If you only want to migrate the history for the last year, you can additionally set a start time" or "If you had trouble setting up Icinga DB in the first place, please double check that the time stamp obtained from the database makes sense".

@julianbrost
Copy link
Contributor

I've pushed a suggestion with how to improve the docs to the feature/migrate-history-docs branch.

While writing the documentation, I asked myself why we even need to ask the user to provide an endpoint name. All endpoint_id columns in the history tables are nullable so it should be perfectly fine to just write NULL there (needs checking that web handles this correctly). AFAIK this column isn't really used much anyways and I don't think it would hurt if it could be used to distinguish migrated events from native events.

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

I'll merge this now as-is as there are no real blockers in here anymore so that we can go on with other changes without introducing merge conflicts with this huge PR all the time. I'll open a new PR for the outstanding details.

@julianbrost julianbrost merged commit 9fb9a10 into master Nov 2, 2022
@julianbrost julianbrost deleted the feature/migrate-history branch November 2, 2022 11:16
@oxzi oxzi added the area/migrate cmd/icingadb-migrate label Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/migrate cmd/icingadb-migrate cla/signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants