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

Audit triplestore #6

Merged
merged 7 commits into from Apr 13, 2015
Merged

Conversation

mohideen
Copy link
Contributor

Functional workflow: Camel picks up JMS events from fedora and updates Fuseki triplestore.

@awoods
Copy link

awoods commented Apr 10, 2015

I assume mvn clean install fails for you, @mohideen, as well?

@mohideen
Copy link
Contributor Author

Yes, I have been skipping tests for quick checks. Missed to run the tests after finishing the work. Will update with fixes for the tests.

@@ -12,7 +12,11 @@
<cm:default-properties>
<cm:property name="jms.brokerUrl" value="tcp://localhost:61616"/>
<cm:property name="jms.fcrepoEndpoint" value="topic:fedora"/>
<cm:property name="triplestore.baseUrl" value="localhost:3030/ds/test"/>
<cm:property name="triplestore.baseUrl" value="localhost:3030/fedora4"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit to having two properties? Why not just make triplestore.baseUrl localhost:3030/fedora4/update?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed -- this should be a single property

@escowles
Copy link
Contributor

:+1 this looks good to me & is working for me locally.

@escowles
Copy link
Contributor

Wait, looks like there's a merge conflict in the RouterTest -- can you rebase this?

@mohideen
Copy link
Contributor Author

Sure, I will merge fcrepo-camel-toolkit:master to PR branch.

@escowles
Copy link
Contributor

👍 now it merges cleanly too – thanks @mohideen

escowles added a commit that referenced this pull request Apr 13, 2015
@escowles escowles merged commit 76921b3 into fcrepo-exts:master Apr 13, 2015
@@ -62,7 +61,9 @@
*/
public void process(final Exchange exchange) throws Exception {
final Message in = exchange.getIn();
final UriRef eventURI = new UriRef("XXX");
final String eventURIBase = (String) exchange.getProperty("eventURI.base");
Copy link

Choose a reason for hiding this comment

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

Does a mixed-case, String-literal property not seem risky?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not particularly risky given that it's set in the EventRouter class from a property placeholder value. But wouldn't it make more sense to put the value in a header? (I realize that the distinction b/t header and property is a bit thin)

Copy link

Choose a reason for hiding this comment

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

I will leave the question of property vs. header up to you...
We can certainly leave eventURI.base as it is; however, I notice an existing convention along the lines of:

  • triplestore.baseUrl

Close, but different.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the property names should follow a more consistent naming pattern

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.

None yet

4 participants