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.

Sign up for GitHub

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

Jump to bottom

geoip2: watch and update #128

Merged
merged 5 commits into from
Oct 31, 2021
Merged

geoip2: watch and update #128

merged 5 commits into from
Oct 31, 2021

Conversation

tydavis
Copy link
Contributor

@tydavis tydavis commented Oct 11, 2021

Add goroutine to watch file modification time each minute. Lock and
update the files independently if the modification time is newer than
the last time it was loaded.

Also ancillary fixes for type signatures, and formatting test files.

Fixes #116, #16

@tydavis tydavis marked this pull request as ready for review October 12, 2021 17:51
abh
abh requested changes Oct 12, 2021
View reviewed changes
targeting/geoip2/geoip2.go Outdated Show resolved Hide resolved
targeting/geoip2/geoip2.go Outdated Show resolved Hide resolved
targeting/geoip2/geoip2.go Outdated Show resolved Hide resolved
targeting/geoip2/geoip2.go Show resolved Hide resolved
@tydavis
Copy link
Contributor Author

tydavis commented Oct 15, 2021

@abh - made both changes per your recommendations. Let me know if you have additional feedback.

@abh
Copy link
Owner

abh commented Oct 16, 2021

Thanks @tydavis! I had two more comments (I forgot how GitHub works and put the comments on the commit, I hope it's not too confusing).

@tydavis
geoip2: watch and update
0b19890 
Add goroutine to watch file modification time each minute. Lock and
update the files independently if the modification time is newer than
the last time it was loaded.

Also ancillary fixes for type signatures, and formatting test files.

Fixes #116, #16
@tydavis
geoip2: add read lock to GetLocation
d5ab161 
bug: read lock missing from GetLocation

GetLocation operates on a *geoip2.City (pointer) but does not incur a
read-lock on the GeoIP struct like other functions.
@tydavis
geoip2: remove select default, use defer unlock
654f28d 
- Remove default execution in the for-select loop with the watchFiles
  ticker (per feedback in PR #128)
- Migrate the DB-reload code into its own function to cleanly use the
  defer-unlock pattern (also per feedback in #128)
@tydavis
Copy link
Contributor Author

tydavis commented Oct 19, 2021

@abh the latest update:

  • Rebases main on to the PR branch
  • Consolidates open such that it is now open-or-update
  • Makes the Has... functions faster by storing active as a boolean when opening the DBs
  • Opens all DBs when New is called
  • watchFiles automatically opens databases if discovered at runtime.

I have tested these operations locally and have verified each of my statements above. I look forward to your feedback.

@tydavis
geoip2: consolidate open, open all DBs
749e8ce 
Per feedback in PR #424, change functions `open`, `New`, and the GeoIP2
struct to allow for independent lock management, update the file in-place if
`open` is called again, and remove enums and other references in the
package.
@tydavis
geoip2: add active-checks to queries
a510b96 
Add checks for the DB to be active before making a query. This was only
enabled on ASN, now enabled for City and Country queries.
@tydavis
Copy link
Contributor Author

tydavis commented Oct 20, 2021

I forgot that the get function would load and error check getting the reader... now we check that the reader is active before querying

@abh
Copy link
Owner

abh commented Oct 20, 2021

Thank you again for all this. The last days were busy; but I'll give it a look and get it merged soon!

@tydavis
Copy link
Contributor Author

tydavis commented Oct 25, 2021

@abh bumping this for review. 😄

@abh abh merged commit a510b96 into abh:main Oct 31, 2021
@abh
Copy link
Owner

abh commented Oct 31, 2021

It looks good, thank you again @tydavis! I've merged it and a couple of the pool.ntp.org servers are running the update now.

@tydavis tydavis deleted the geoip_concurrentUpdate branch November 2, 2021 18:09
@abh abh mentioned this pull request Jan 20, 2022
Closed
bufrr pushed a commit to bufrr/geodns that referenced this pull request Feb 1, 2023
@tydavis @bufrr
geoip2: remove select default, use defer unlock
8906510 
- Remove default execution in the for-select loop with the watchFiles
  ticker (per feedback in PR abh#128)
- Migrate the DB-reload code into its own function to cleanly use the
  defer-unlock pattern (also per feedback in abh#128)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abh abh abh requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Will the geoip database be automatically reloaded every day?
2 participants
@tydavis @abh