908798 - New middleware_app should stop using "hbase" and "filesystem" in favor of "hb" and "fs"

Status: RESOLVED FIXED

(Socorro Graveyard :: Middleware, defect, P3)

Description

[Peter Bengtsson [:peterbe]]

We need to replace 'socorro.external.filesystem' with 'socorro.external.fs' and replace 'socorro.external.hbase' with 'socorro.external.hb'.

Changing it is easy. Testing it is not.

Comment 1

[[DEACTIVATED] Adrian Gaudebert]

I started working on that by replacing all instances of filesystem.FileSystemCrashStorage by fs.FSLegacyRadixTreeStorage. I am now stuck and can't get the monitor-integration-test.sh script to pass. It hangs on the monitor, which simply doesn't detect any crash coming. The collector test passed, the processor is running. 

Lars, did we ever test the configmanized middleware with that new `fs` module? It's hard for me to debug that, having no experience with that part of Socorro, and the logs don't show any error.

Comment 2

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

Here is my current diff. I also reset all the config files to use the default values.

Comment 3

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 794934 [details] [diff] [review]
patch.diff

r-

the collector_app crashstorage_class should be 'socorro.external.fs.crashstorage.FSLegacyDatedRadixTreeStorage'

the middleware change is correct

the monitor_app crashstorage_class should be 'socorro.external.fs.crashstorage.FSLegacyDatesRadixTreeStorage' 

the processor_app crashstorage is correct


In each case where a class is specified, the python file should not import the class and then specify it as the default on the option.  Remove the imports and give the full classpath as a string to the default:

yes:
    required_config.add_option(
        'crashstorage_class',
        doc="the class of the crashstorage system",
        default='socorro.external.fs.crashstorage.FSLegacyDatesRadixTreeStorage'
    )

no:
    from socorro.external.fs.crashstorage import FSLegacyRadixTreeStorage
     required_config.add_option(
        'crashstorage_class',
        doc="the class of the crashstorage system",
        FSLegacyRadixTreeStorage
    )

the latter causes a dependency in the Python file that is useless if the value is overridden in an ini file.  The former doesn't introduce the dependency.

Comment 4

[K Lars Lohn [:lars] [:klohn]]

s/Dates/Dated

Comment 5

[Peter Bengtsson [:peterbe]]

Maybe this comment is little off-topic but I really don't want the switch to the new middleware_app to ALSO mean changing which hbase classes and which file system classes we use. That would mean having to switch on potentially 3 new systems all at the same time. 

For what it's worth, I think we should enable the new middleware on stage and prod using whatever classes we currently use for crashstorage on stage and prod.

Comment 6

[[DEACTIVATED] Adrian Gaudebert]

Peter, those are two separate issues. We can ship the configmanized middleware now. And what we are changing here are _defaults_, that will not have any impact on our dev/stage/prod systems, were we overwrite those values. The only ones who will be impacted are external users installing Socorro for the first time.

Comment 7

[Peter Bengtsson [:peterbe]]

:adrian, can the defaults left in middleware_app.py match what we have in stage/prod. That way we don't have to A) switch to middleware_app.py and B) edit our puppetized middleware.ini to adjust.

Comment 8

[[DEACTIVATED] Adrian Gaudebert]

Not really, we want Socorro to be configured as simply as possible for new installs. So a default instance of Socorro should only require postgres and the filesystem, where one of our installs will also elasticsearch and HBase. 

However, the changes there are really small, it's basically just the databases credentials and a few service implementation overwriting (Search with elasticsearch for example). 

(In reply to Peter Bengtsson [:peterbe] from comment #7)
> That way we don't have to A) switch to middleware_app.py and B)
> edit our puppetized middleware.ini to adjust.

I'm not exactly sure what you mean there.

Comment 9

[Peter Bengtsson [:peterbe]]

I appreciate the importance of making the defaults ideal for easy deployment. But not now. For now, when we switch to middleware_app I want the defaults to reflect the currently used crashstorage classes so that there's less that needs to change. 

Later we can change the defaults and change middleware.ini (in puppet) so that it uses the new hb and the new fs.

Comment 10

[[DEACTIVATED] Adrian Gaudebert]

I don't understand your point, Peter. You want to make some changes to the code base so that the defaults reflect what we use in stage / prod, to later revert those changes and introduce some changes in our ini files. Why not just make the ini files right now? It's just way simpler, doesn't break anything for developers or external users, and doesn't involve more work later on (that we are likely to forget).

Comment 11

[[DEACTIVATED] Adrian Gaudebert]

This was resolved with bug 948533.