Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#14024 closed enhancement (fixed)

web-app configuration error handling

Reported by: iwakeh Owned by:
Priority: Low Milestone:
Component: Metrics/Onionoo Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I suggest for DocumentStore.setOutDir checking if the out directory exists
and throwing an error if not.

In (the very likely) case that someone doesn't follow the INSTALL file
suggestion to use '/srv/onionoo' as working directory, the missing folder
is reported immediatly.

And, maybe add some hint about changing 'etc/web.xml.template' to the top
of INSTALL? Like "If you choose a different working directory make sure
its path is reflected in the following files: ..."

Child Tickets

Attachments (1)

0001-task-14024-adding-dramatic-error-message-for-missing.patch (3.3 KB) - added by iwakeh 4 years ago.

Download all attachments as: .zip

Change History (8)

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

Replying to iwakeh:

I suggest for DocumentStore.setOutDir checking if the out directory exists
and throwing an error if not.

In (the very likely) case that someone doesn't follow the INSTALL file
suggestion to use '/srv/onionoo' as working directory, the missing folder
is reported immediatly.

Yes, good idea. Want to submit a patch for this?

And, maybe add some hint about changing 'etc/web.xml.template' to the top
of INSTALL? Like "If you choose a different working directory make sure
its path is reflected in the following files: ..."

Or should we take this parameter out of etc/web.xml.template? We could default to out/ in the current working directory, and accept a command-line parameter to override that default similar to the logging directory. Does that make sense?

comment:2 Changed 4 years ago by iwakeh

I think it is most useful that the setOutdir method just verifies the path and
throws FileNotFoundEx. if there is no directory.
The callers have to deal with that. Currently, only NodeIndexer is affected and
an informative message should be logged and the web application should exit.

A patch is attached, which exits the web application and logs an error message and
a hint about where to fix the path, i.e.

        Out-dir not found! Expected directory: /srv/onionoo.torproject.org/onionoo/out
        Verify the configuration in ./etc/web.xml.template

I think it might be better to keep real application configuration in files rather than
on the command line. That way it'll be easier to determine runtime parameters during trouble-shooting.
Command lines can be ephemeral sometimes.

If the location of the outdir configuration is changed in future, the error handling
is close to the affected code and can be easily adapted.

comment:3 Changed 4 years ago by iwakeh

Status: newneeds_review

comment:4 Changed 4 years ago by karsten

Status: needs_reviewnew

Patch looks good. Merged to master. Thanks!

I'm yet undecided if we should find a better solution than defining the directory in etc/web.xml which is then included in the .war file. If we ever want to give out binaries, which include the .war file, people wouldn't be able to change the path to their out/ directory. If command-line parameters are not the way to go, how about we use something else for config files? (I vaguely remember that you suggested something on a different ticket, but I don't remember the details.)

comment:5 Changed 4 years ago by iwakeh

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.

comment:6 Changed 4 years ago by karsten

Resolution: fixed
Status: newclosed

Either commons-configuration or properties file works for me. And yes, new ticket makes sense. Want to create one and ideally submit a patch? Resolving this issue as implemented. Thanks!

comment:7 Changed 4 years ago by karsten

Created #14201 for configuring the out/ directory path.

Note: See TracTickets for help on using tickets.