Opened 4 years ago

Closed 20 months ago

#14201 closed enhancement (implemented)

Configure out/ directory path somewhere else than in web.xml.

Reported by: karsten Owned by: iwakeh
Priority: Medium Milestone: Onionoo-1.4.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Quoting iwakeh from #14024:

You're right web.xml is an odd place for setting the out dir path.

In #13600 I mentioned commons-configuration (but there too a config file would be better than command line, I think). User docs are here ​Commons Configuration User Guide

Without an additional lib reference, why not use a plain old java properties file?
Have useful defaults for everything and write the properties file, if the app could not
find one initially?

This ought to be a new issue.

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by iwakeh

What else should got into the properties file?

Format suggestion:

##
# onionoo properties
##
onionoo.server.out=/some/path/out
onionoo.docs.xyz=yyy

comment:2 in reply to:  1 Changed 4 years ago by karsten

Replying to iwakeh:

What else should got into the properties file?

How about:

  • path to out/ directory (relevant for both back-end and front-end),
  • path to status/ directory (only relevant for back-end),
  • path to logging directory (relevant for both),
  • boolean for server maintenance mode (front-end only).

Format suggestion:

##
# onionoo properties
##
onionoo.server.out=/some/path/out
onionoo.docs.xyz=yyy

Format looks good.

comment:3 Changed 2 years ago by iwakeh

Severity: Normal
Status: newneeds_information

This can be easily derived from the CollecTor configuration process (cf. o.t.c.conf package, classes Key and Configuration), but without any runtime-update.
Without logging path, because that's part of the logging framework and outside of the application.

Could be candidate for first release, too.

Thoughts?

comment:4 Changed 2 years ago by karsten

Yes, sounds good!

comment:5 Changed 21 months ago by iwakeh

Milestone: Onionoo-1.3.0
Owner: set to iwakeh
Status: needs_informationaccepted

comment:6 Changed 21 months ago by iwakeh

Milestone: Onionoo-1.3.0Onionoo-1.4.0

comment:7 Changed 20 months ago by iwakeh

Status: acceptedneeds_review

Please review this patch.

I decided to only use a system property onionoo.basedir. If it is set on the java command line, the given path will be the base for all of Onionoo's paths (out, in, status).

The configuration options pondered above all seem too big for simply providing one property.

comment:8 Changed 20 months ago by karsten

Status: needs_reviewneeds_revision

Hmm, there's one major issue with this approach: This change is backward-incompatible with respect to the hourly updater, which previously used "." as base directory rather than "/srv/onionoo.torproject.org/onionoo".

And another, minor issue is that we're now defining that path in multiple places. This seems bad for future maintenance, in particular if we ever want to rename Onionoo and update that path.

How about we limit this change to just the server part and leave the hourly updater unchanged. That would implement this ticket. I could edit the patch, unless you'd rather want to do that.

comment:9 Changed 20 months ago by karsten

I'll prepare a patch for the smaller change in order to save an iteration in this review process.

comment:10 Changed 20 months ago by karsten

Status: needs_revisionneeds_review

Please review my task-14201 branch.

comment:11 in reply to:  10 Changed 20 months ago by iwakeh

Status: needs_reviewmerge_ready

Replying to karsten:

Please review my task-14201 branch.

This looks fine. The old approach was too eager and limiting this to the web component is better.

One tiny thing in 'Main':

-  private Main() {/* empty */}
+  private Main() {
+  }

The coding guidelines state that empty blocks may be concise. I would prefer the comment or {} without comment to the dangling brace, which looks as if something was missing.

Other than this, merge ready.

comment:12 Changed 20 months ago by karsten

Resolution: implemented
Status: merge_readyclosed

Sounds good. Merged with {}. Closing.

Note: See TracTickets for help on using tickets.