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

12311 auto memory docs #819

Merged
merged 19 commits into from Jul 10, 2014
Merged

12311 auto memory docs #819

merged 19 commits into from Jul 10, 2014

Conversation

joshmoore
Copy link
Member

Documentation PR for ome/openmicroscopy#2595

This PR should initially be used for a clarification of the code in 2595, but should remain open for the likely numerous adjustments which will be necessary in the code. Built documentation should be visible under:

Both the sections:

have been updated to point to adv. configuration.

Now that the default memory configuration need not
be modified by default, it's less critical that the
info be embedded in the main installation page. Also,
this text will need to grow substantially in order to
make the usage clear, and repeating that between unix
and windows seems error prone.
^^^^^^^^^^^

A number of configuration properties starting with ``omero.mem``
control how OMERO calculate how much memory to allocate to each
Copy link
Member

Choose a reason for hiding this comment

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

calculate -> calculates

Copy link
Member

Choose a reason for hiding this comment

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

you don't need the 'each of'

@joshmoore
Copy link
Member Author

All noted and appreciated. I'll correct tomorrow morning. There's a lot more verbiage needed, though. So if you and/or other people who shall remain nameless want to hold off on the review, I'll do multiple readings myself before signing off.

@sbesson
Copy link
Member

sbesson commented Jun 17, 2014

Talked with @joshmoore. Excluding to let #826 in.

@manics
Copy link
Member

manics commented Jun 17, 2014

The docs mention blitz, indexer, pixeldata, but the generated config also has repository. Worth a mention even if it's of lesser importance?

The installation docs now just say "To increase the performance of your system, you may want to tune the JVM". Is it worth emphasizing this a bit more?

@manics
Copy link
Member

manics commented Jun 17, 2014

@joshmoore
Copy link
Member Author

All noted. Once the code PR is finalized, I'll update this with the suggestions.

@sbesson sbesson removed the exclude label Jun 18, 2014
@sbesson
Copy link
Member

sbesson commented Jun 18, 2014

Now #826 has been merged, this will need origin/dev_5_0 merged in.

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-5.0-merge-docs#731. See the console output for more details.

@joshmoore
Copy link
Member Author

Excluded until the code PR is merged.

@joshmoore joshmoore removed the exclude label Jun 23, 2014
@joshmoore
Copy link
Member Author

exclude removed. origin/dev_5_0 has been merged, and the latest state of ome/openmicroscopy#2595 at least roughly outlined.

@mtbc
Copy link
Member

mtbc commented Jun 25, 2014

That's all I notice on a brief read-through anyway. (-:

@hflynn
Copy link
Member

hflynn commented Jun 30, 2014

I think it might be easier to do the re-organisation on another PR - I would like to spend a bit of time planning out new section divisions for the sysadmin docs this week. Given that, it may be better to leave the cross-links until then. Don't know if you want to add a table on this PR or later @joshmoore ?

Configuration properties can either be applied to all three service
types at the same time by omitting the service type (e.g.
:term:`omero.mem.strategy`) or to each individually by including it
(e.g. `omero.mem.blitz.strategy`).
Copy link
Member

Choose a reason for hiding this comment

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

No idea how messy this would look but Sphinx allows to define multiple terms in glossary e.g.

.. glossary::

    omero.mem.strategy
    omero.mem.blitz.strategy
    omero.mem.indexer.strategy
    omero.mem.pixeldata.strategy
        Selects which memory strategy to use.

@hflynn
Copy link
Member

hflynn commented Jul 4, 2014

#844 is merged now @joshmoore so you can merge dev_5_0 into this and continue working - just put the new subdivided configuration pages in the 'Optimizing Server Configuration' section with nice clear explanatory titles.

@joshmoore
Copy link
Member Author

My tendency would be to merge this as close to it as and then handle restructuring with search, etc.

@hflynn
Copy link
Member

hflynn commented Jul 9, 2014

Sorry, I'm not clear - do you want me to merge this now as is? Or are you doing some more work on it first?


::

$ bin/omero config set omero.mem.strategy adaptive
Copy link
Member

Choose a reason for hiding this comment

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

not any more....

@bpindelski
Copy link

In general, this looks OK. Still some minor details to iron out. Once this is fully in-line with what the code does, this is good to merge.

http://www.openmicroscopy.org/community/viewtopic.php?f=4&t=7400
Forum thread on PixelData JVM memory settings

:ref:`gridconfiguration`
Copy link
Member

Choose a reason for hiding this comment

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

Having a highlighted box with See also referring to the section immediately below looks a bit strange. Not a blocker for merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see if that goes away in the re-working.

@joshmoore
Copy link
Member Author

Another round of fixes pushed.

@manics
Copy link
Member

manics commented Jul 10, 2014

👍
@hflynn Can you merge this before someone else comments :)

hflynn added a commit that referenced this pull request Jul 10, 2014
@hflynn hflynn merged commit 52531fc into ome:dev_5_0 Jul 10, 2014
@joshmoore
Copy link
Member Author

--rebased-to #869

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants