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

RYA-70 web.rya spring xml files don't pull in environment properties … #317

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brushworth
Copy link

@brushworth brushworth commented Jul 29, 2020

…consistently to a well-defined standard. This commit begins to tidy the Accumulo config (MongoDB to come) but more work is required.

Description

What Changed?

The current structure and documentation concerning the environment.properties and spring xml configuration seems to be lacking. Trying to get advanced features working in Tomcat, for example the AccumuloSelectivityEvalDAO or the various indexing strategies, is a very hard slog, particularly for new comers to the project.

I've started to tidy up the accumulo and extension spring xml files. I've tested the accumulo and extensions files on a test cluster.

In future, I propose we add default properties files to the project that work out of the box with Fluo Muchos (or similar) to allow for easy spin up of a development cluster to give Rya a whirl, for example on AWS or Azure. People can then easily edit them to their environment, rather than having to reverse engineer what parameters are available and what default values are in use.

I'm happy to do MongoDB configuration too but I don't have a test cluster running at present.

I'm after feedback from more experienced developers of Rya about whether these changes are heading in the correct direction. For example, I've replaced some of the configuration xml calling setter methods like this:

    <bean id="conf" class="org.apache.rya.accumulo.AccumuloRdfConfiguration">
        <!-- Calls setter method name -->
        <property name="tablePrefix" value="${rya.tableprefix}"/>
        <property name="displayQueryPlan" value="${rya.displayqueryplan}"/>
        <property name="useStats" value="false"/>
        <property name="useStats" value="${rya.usestats}"/>
        <property name="useSelectivity" value="${rya.useselectivity}"/>
        <property name="useStatementMetadata" value="${rya.usestatementmetadata}"/>
        <property name="numThreads" value="${rya.querythreads}"/>
        <property name="batchSize" value="${rya.batchsize}"/>
        <property name="dataWaveEdge" value="${rya.datawaveedge}"/>
        <property name="dataType" value="org.eclipse.rdf4j.model.Statement"/>
        <!--
        <property name="useEntity" value="${sc.use_entity}"/>
        <property name="useGeo" value="${sc.use_geo}"/>
        <property name="useFreeText" value="${sc.use_freetext}"/>
        <property name="useTemporal" value="${sc.use_temporal}"/>
        -->
    </bean>

with

    <bean id="conf" class="org.apache.rya.accumulo.AccumuloRdfConfiguration" factory-method="fromProperties">
        <constructor-arg ref="properties"/>
    </bean>

Additionally, there is a lot of duplication in the properties space. I'm trying understand why. I'm also trying to understand the differences.

For example, there are environment properties accumulo.instance, instance.name and sc.cloudbase.instancename in different places, all obviously referring to the same type of thing? Is this redundancy deliberate, or can I start consolidating it down? Can we get down to a single Rya configuration properties file for an entire Rya installation (e.g. ingest jobs, Tomcat, Fluo, etc)?

I'm still a little lost in the details here, and any background or advice would be much appreciated. It feels like a bit of a mess at the moment.

This pull request is ready to go and makes things a fair bit tidier, but I'm happy to put up a subsequent one taking these ideas further as the community provides feedback,

Tests

Coverage?

N/A

Links

Jira RYA-70

Checklist

  • Code Review
  • Squash Commits

People To Reivew

[Add those who should review this]

…consistently to a well-defined standard. This commit begins to tidy the Accumulo config (MongoDB to come) but more work is required.
@brushworth brushworth marked this pull request as ready for review November 16, 2020 03:09
@brushworth
Copy link
Author

This change does nothing other than tidy up the properties to make it easier for new users. There is also a name change of the default testing namespace to prevent any accidents from rya_ to ryatest_.

@DLotts
Copy link

DLotts commented Nov 17, 2020 via email

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

Successfully merging this pull request may close these issues.

2 participants