Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19021 closed enhancement (fixed)

improve configuration process

Reported by: iwakeh Owned by: iwakeh
Priority: High Milestone: CollecTor 1.0.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: ctip operation
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Sketch of the new configuration process:

  • use Properties to store and read the configuration parameters.
  • read external configuration properties file (either local path or via command line argument).
  • provide a default configuration as resource within the executable jar.
  • if there is no external configuration file present, write out the default values.
  • re-read properties when file changes; only between module runs (this is after the implementation of the scheduler, #19018)
  • use enum as keys

Questions to decide:

  1. property format: XML or plain properties (the latter might be more readable)
  2. default value settings

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by iwakeh

Priority: MediumHigh

set prio to high as it blocks #19018.

comment:2 in reply to:  description Changed 3 years ago by karsten

Sounds like a good plan to me. Regarding your questions, I'd say let's go with plain properties rather than XML and with default values from config.template, if possible.

comment:3 Changed 3 years ago by iwakeh

Does the following topic from the RoadMap mean that Torperf data fetching will be obsolete in CollecTor then?
Take Torperf offline; karsten; 2016-09-30

comment:4 in reply to:  3 Changed 3 years ago by karsten

Replying to iwakeh:

Does the following topic from the RoadMap mean that Torperf data fetching will be obsolete in CollecTor then?
Take Torperf offline; karsten; 2016-09-30

Possibly, yes. The plan is to switch from Torperf to OnionPerf, and OnionPerf might come with its own code to fetch data from other OnionPerf instances and make that available to the world. But I'm not entirely certain which role CollecTor will play once OnionPerf is in place. We might be able to remove the code that merges .data and .extradata files, but we might need to keep the Torperf results archiving code. I'd say don't give up on the Torperf module in CollecTor just yet.

comment:5 Changed 3 years ago by iwakeh

Status: newneeds_review

Configuration process is changed:

  • running the jar without any args will write the default properties files with the old defaults
  • all properties are set, i.e. no 'hidden' defaults
  • a properties file can be specified as second command line argument, if not, collector.properties in the current folder is used.
  • Configuration reads the properties and verifies there syntax, e.g. bool needs to be 'true' or 'false'.
  • A Key enum contains all the property keys.

In addition:

  • Tests added for all new classes; and an overall smoke-test added to also verify the new configuration. (The tests are not allowed to contact the network, so they won't bother any production servers.)
  • Coverage for new classes up to 100%.
  • Removed System.exits to make CollecTor more testable and in preparation for scheduler task.
  • ...

Please review and test thoroughly. my branch
In addition, I'll also test this version on the mirror.

comment:6 Changed 3 years ago by iwakeh

errata:

  • running without args gives the usage
  • running with a module specified, but either no default or missing or empty properties file will write default properties.

comment:7 Changed 3 years ago by iwakeh

Status: needs_reviewneeds_revision

I just noticed some paths I missed.

Setting to needs_revision.

comment:8 Changed 3 years ago by iwakeh

Status: needs_revisionneeds_review

Please review the corrected new branch.

I had this running for quite a few hours on the mirror; it produces the same directories as master.

Once it is reviewed, I'd suggest waiting for the merge to master and merge it together with the (still open) scheduler implementation.

Thanks!

Version 0, edited 3 years ago by iwakeh (next)

comment:9 Changed 3 years ago by karsten

Status: needs_reviewmerge_ready

Alright, I made it through that patch and ran some local tests. Surprisingly (to myself) I don't have a single change request! I briefly thought whether CollecTor should really write the collector.properties file if it doesn't exist, because that's a bit unexpected, but I couldn't think of a situation where that would be harmful.

A minor remark though for future patches of similar size: Can you split up big commits that change more than one thing into several smaller commits? Whenever I find myself writing a huge patch, I go through the diff and try to find related changes and give them numbers. Then I do git reset HEAD^ and run git add -p and git commit until all changes are committed. There are more advanced ways to split up commits using git rebase -i, but in this case that would not have been necessary. The beauty of smaller commits is that they're easier to review. Though I don't know whether splitting it up would have resulted in several commits of similar size or one almost-as-big commit and a couple of tiny ones. See the recent Javadoc change to metrics-lib where I separated even tiny code changes from the Javadoc commit.

So, should I still wait with merging this to master? Do you expect this commit to change once the scheduler implementation gets in? If not, why not merge now?

comment:10 Changed 3 years ago by iwakeh

Thanks for looking at this so soon!

The properties file will be explained in the future operation instructions. So, it shouldn't be confusing, I hope.

I totally agree to have handy commits. But I'm very reluctant to commit any code that doesn't
compile or fails tests. So, I couldn't find a shorter way of committing it w/o introducing some
weirdness like having both Configuration.java classes in different packages or similar.

Regarding the merge:
We don't have a release yet and the configuration change is not reflected in the shell scripts,
that's why I would not merge it to collector.git master branch.

Instead, maybe add this branch to collector.git and once there is a scheduler patch branch based
on this branch, both can be merged to the main collector.git master branch?
Once the branch is added to the main collector.git this ticket could be closed as implemented.

Last edited 3 years ago by iwakeh (previous) (diff)

comment:11 in reply to:  10 Changed 3 years ago by karsten

Replying to iwakeh:

[...] But I'm very reluctant to commit any code that doesn't
compile or fails tests.

Right, I totally didn't want to suggest that. It's the minor changes like using Paths instead of File that could have moved to separate commits. But it's fine as it is now.

Regarding the merge:
We don't have a release yet and the configuration change is not reflected in the shell scripts,

Right.

Instead, maybe add this branch to collector.git and once there is a scheduler patch branch based
on this branch, both can be merged to the main collector.git master branch?
Once the branch is added to the main collector.git this ticket could be closed as implemented.

There's a small problem with that: Once we push something to collector.git, it'll stay there forever, or until we ask a Git admin to remove it. Instead, I'll add a signed note below that I reviewed your branch so that I can easily merge or cherry-pick it later when we merge the scheduler patch branch.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I reviewed commit 8767c73d0826dfa9aa21e70a2d857c8a2d77e524 of branch
task-19021-improve-conf-process in repository
user/iwakeh/collector.git and suggest merging that to collector.git
master once the scheduling patch branch is ready.

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQEcBAEBAgAGBQJXUEV8AAoJEC3ESO/4X7XBSvUIAJZuyJTi/lvcUSwSJXY6QgyV
A8QXGp3CEdROrqbc1fOtvKiOa4FIC5GHOr78BT1FAYuMDgjHrss6po05RwSNaKLX
OeqzGmAF59PFJYmaJ1HidkTlKf1lJneEEveSY9UlVwVWlPlC+W69T8i4WxxbI1Xo
E4oThC47VzyM1Oc+CQ+xAfW/rpH138Z+ntEjIHBtdKoWzS7hBzlNtc9prM8oKIuj
kKKwvZuQQI5SclhEYmJHELBlnlSQ2YTepkMfUCBXlLq3S4WhN/K9tsL5KmlKIW9C
sQv+Qpo4ukaH7rVNCmHZOSov8D3sM+r2bRWW87utCzsKWCjJx5u+SIVPITXzAWg=
=Fhxs
-----END PGP SIGNATURE-----

comment:12 Changed 3 years ago by iwakeh

Is the branch deletion only allowed in user repos and blocked by a hook in the main git repo?

comment:13 Changed 3 years ago by Sebastian

Yes, official repositories don't support removal of history.

comment:14 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

8767c73d0826dfa9aa21e70a2d857c8a2d77e524 is now merged to master. Closing. Thanks!

comment:15 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.0.0

Added to milestone for first release.

Note: See TracTickets for help on using tickets.