Skip to content

JENA-1960: Clean up Fuseki dispatch#794

Merged
afs merged 4 commits into
apache:masterfrom
afs:fuseki-new-config
Sep 11, 2020
Merged

JENA-1960: Clean up Fuseki dispatch#794
afs merged 4 commits into
apache:masterfrom
afs:fuseki-new-config

Conversation

@afs
Copy link
Copy Markdown
Member

@afs afs commented Sep 9, 2020

Covers:

JENA-1960 Clean up Fuseki dispatch

and the small:

JENA-1961 Support /$/metrics in Fuseki Main
JENA-1962 Assembler for a dataset of a single graph taken from another dataset.


/**
* Old style compatibility.
* For each endpoint in "endpoints1", ensure there is a endpoint on the dataset 9endpoint name "") itself.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"an endpoint", and s/9endpoint/(endpoint.

Copy link
Copy Markdown
Member

@rvesse rvesse left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can follow the changes, some minor comment nits

}

/** Register an addition assembler */
/** Register an addition assembler */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

additional ?

accEndpointOldStyle(endpoints1, Operation.GSP_R, fusekiService, pServiceReadGraphStoreEP);
accEndpointOldStyle(endpoints1, Operation.GSP_RW, fusekiService, pServiceReadWriteGraphStoreEP);

// ---- Legacy for old style: a request wouls also try the dataset (i.e. no endpoint name).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wouls -> would

if ( endpoint == null )
// Named service, no such endpoint.
// Allows for resources under /dataset/
// Does to Jetty's default handling (404 for GET, 405 other methods).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does -> Goes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is actually out of date. This PR also changes the default handling of this case, replacing the Jetty default handling because Jetty is different to other servers.

* nor that access control will allow it to be performed.
*/
public static Operation chooseOperation(HttpAction action) {
private static Operation chooseOperation(HttpAction action) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any particular reason to lock things down to private ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Only that this is an internal operation. From what I can tell, it is a left over from when dispatch was split across multiple classes.

@afs afs force-pushed the fuseki-new-config branch from c4c5d8b to 506a188 Compare September 10, 2020 09:28
@afs afs force-pushed the fuseki-new-config branch from 506a188 to 9c7aad3 Compare September 10, 2020 09:36
@afs
Copy link
Copy Markdown
Member Author

afs commented Sep 10, 2020

as far as I can follow the changes

Yes! Dispatch, in all its cases and all its extensibility is complicated. It's been though a number of rewrites. The test coverage is good and that has been essential in this PR which is a simplification of the dispatch process.

Thanks to you both for looking.

@afs afs merged commit 16072d3 into apache:master Sep 11, 2020
@afs afs deleted the fuseki-new-config branch September 11, 2020 15:40
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.

3 participants