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

cadc-11115 new caom2-meta-sync module #247

Merged
merged 15 commits into from
Jun 15, 2023
Merged

Conversation

jburke-cadc
Copy link
Contributor

No description provided.

Copy link
Member

@pdowler pdowler left a comment

Choose a reason for hiding this comment

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

Haven't looked at Main yet; config/doc changes first and that will reduce what's in Main

caom2-meta-sync/README.md Outdated Show resolved Hide resolved
caom2-meta-sync/README.md Outdated Show resolved Hide resolved
caom2-meta-sync/README.md Show resolved Hide resolved
caom2-meta-sync/README.md Outdated Show resolved Hide resolved
caom2-meta-sync/README.md Show resolved Hide resolved
caom2-meta-sync/build.gradle Outdated Show resolved Hide resolved
caom2-meta-sync/build.gradle Outdated Show resolved Hide resolved
caom2-meta-sync/build.gradle Outdated Show resolved Hide resolved
@pdowler pdowler marked this pull request as draft June 21, 2022 20:49
Copy link
Member

@pdowler pdowler left a comment

Choose a reason for hiding this comment

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

README and configuration:

Until we get a response from Job on the db, let's simplify as much as possible:

  1. get rid of dryrun - it's useless
  2. change runContinuously to exitWhenComplete, make it optional, and default to false: that way run forever is the normal mode and this is an exception for special cases
  3. both threads and batchSize are implementation details that we unfortunately expose to operators and explaining sensible values will be very hard... let's make those internal and for now hard code to batchSize=100 and nthreads=batchSize/10.
  4. change maxSleep to maxIdle and keep it with the "source" config items
  5. hide (remove?) the "source db" config documentation for now
  6. use org.opencadc.caom2.metasync.repoService for source config, move the org.opencadc.caom2.metasync.collection up beside the repoService and make it clear that multiple values are allowed; put all "source:" config before "destination" config
  7. use org.opencadc.caom2.metasync.db.* for destination config; keep org.opencadc.caom2.metasync.basePublisherID with destination config

Style: In the text that explains each config property, you don't need to write out the whole property. It's hard to read... see fenwick (trackSiteLocations) or tantar (buckets) for two styles that seem to be sufficient ref to the config spec and more readable (just inconsistent, but I think I prefer the italics since they aren't literally exact).

@jburke-cadc jburke-cadc marked this pull request as ready for review June 15, 2023 13:15
@pdowler pdowler merged commit 9c2fd2a into opencadc:master Jun 15, 2023
1 check passed
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.

None yet

2 participants