Github user kinow commented on the issue:
https://github.com/apache/jena/pull/227
I think only the OP can close it, or a commit mentioning the PR number.
Some time ago in Commons I think I had to close a PR, and I used an empty
commit like this:
git commit --allow-empty
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
This PR can be closed. I don't have sufficient rights to do that and GitHub
didn't notice that the code already got in, probably because I squashed the
commits.
---
If your project is set up for it,
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Thanks Osma for incorporating the changes into master. ð
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
@osma I'm done mumbling over this PR. I think it looks okay. I did leave
some questions about the Maven stuff, but nothing in there makes me freak out.
The biggest question I have is why `log4j` is
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
Thanks for taking a look @ajs6f. I will wait for your review before
proceeding
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
I think that if we are confident that we have solid test coverage and that
it runs correctly, we can merge and clean up as time permits. I will take a
look at the Maven setup and see if anything worries
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
:+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
I tested the ES backend with some non-toy SKOS data, namely
[YSO](http://finto.fi/en/yso/). I configured the entity definition to index the
predicates `skos:prefLabel`, `skos:altLabel` and
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Thanks @osma . Can you point me to the SNAPSHOT repo please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv Thanks for merging master, the diffs are now much cleaner!
Regarding releases: Jena doesn't have scheduled releases. Traditionally
there have been about two releases per year, but
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma I have merged the changes from Master into my branch.
I am fine with merging the code on Monday/Tuesday. Can you also let me know
when will 3.3.0 be released? Currently, to not stop us
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
#226 has been merged. I suggest the following plan for merging this PR:
- @anujgandharv it would be great if you could rebase on top of current
apache master, which includes #226. That would
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Cool.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv Ah, right, sorry, I didn't remember to check that case. Good
thing you got it working! I will proceed with merging #226 first and then let's
get on with merging this one.
---
If your
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma @ajs6f I have made the necessary changes to the ES TextIndex based on
changes in #226
I want to bring one thing to notice:
If the query string is: `?s text:query ('word' 'lang:en'
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma Let me try to merge your changes in #226 to my code and see if I can
turn it around today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
I'm definitely cool with squashing a bit. I think that helps posterity a
lot. Two possible (plausible?) principles:
1. Each post-squash commit should move the codebase from a does-build state
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
Regarding merging, I would like to merge PR #226 first which changes the
`TextIndex.query` method, breaking out the `lang` and `graph` information into
separate parameters. The effect on this code should
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma @ajs6f I have added integration tests for ES based Indexing Strategy.
Could you guys please review and let me know if they are fine and if I missed
anything.
I do not have any more
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Great. Thanks @Osma. I misunderstood your previous comment. I will
implement the integration tests for the above scenarios.
---
If your project is set up for it, you can reply to this email and
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Thanks @osma and @ajs6f for your inputs. Can I then suggest that instead of
moving TestTextIndexES to integration tests module, lets get rid of it
completely and instead have the same tests as
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv That was what I meant - get rid of all the currently written
ES test classes completely by moving all the existing unit tests to the new
integration tests.
I've suggested two sets
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
I don't know enough to have a useful opinion about how thorough the tests
need to be-- I am very happy to defer to @osma about that.
As for the test framework-- my (small) knowledge of ES
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
I suggest you move the existing tests from TestTextIndexES to the
integration tests. The tests that rely on an embedded ES have no future anyway,
since the ES embedded mode is already crippled and is
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv I tested it and got the same error.
I found some hints in this issue report:
https://github.com/elastic/elasticsearch/issues/22494
The problem seems to be that the ES test framework
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
This is the error I am getting
```
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 11.253 sec
<<< FAILURE! - in org.apache.jena.query.text.it.TextIndexESIT
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv I didn't mean that we should introduce a Spring dependency. I
just found the project and thought it could serve as an example for integrating
the ES Maven plugin.
Does this article
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma spring-elasticsearch IT is throwing exactly the same error that I am
getting on my setup. What they have done is they are ignoring all the errors
and assuming there are no errors, thus the
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma @ajs6f I have tried the ES Maven plugin but it is throwing me lots of
errors. I suspect that it is just a wrapper around the embedded elasticsearch.
I asked the creator to provide a more
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
@anujgandharv, the first place to look is at the actual
`jena-integration-tests` module. You will see there that the tests follow the
same form as in other Jena modules (explicitly linked together in
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Thanks @ajs6f for the Maven ElasticSearch Plugin link. Looks like this
would enable us to spin up a fully functional Single Node ES for our
integration tests. Can you share some more light as to
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/227
I'm just catching up with this PR, so apologies if I turn out to have
missed part of the conversation!
If there is difficulty getting tests integrated into the Maven lifecycle
within one
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Thanks @osma. I think the Index will becoe much simpler if we remove the
non-used methods
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
I asked advice about what to do with those methods on the `dev` list:
http://jena.markmail.org/thread/pgeigsya7f5xda6h
---
If your project is set up for it, you can reply to this email and have your
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
Hi @anujgandharv, thanks for the update, I will do a new review soon.
Regarding `TextIndex.get`, excellent question! I was actually wondering the
same when I did the review yesterday, but didn't
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
Hi @osma I need one more favour from you. I need to understand the scenario
when the 'get' method of TextIndex gets called. Can you provide me an example
Sparql query which I can run from my
Github user anujgandharv commented on the issue:
https://github.com/apache/jena/pull/227
@osma Can you review now. I have made the changes to the Add and Delete API
so that they are executed as a single REST call. I think we already have
consensus on the Multilingual aspect. If not,
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
Thanks @anujgandharv for the very quick responses to my comments! Please
let me know when you're done with them all and want a second review.
It would be helpful if your commit messages were a
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
I think it would help the review if you rebased your branch on top of
current apache master. Now it's hard to see what are your ES changes and what
came from the included commits. Dropping Solr in
Github user osma commented on the issue:
https://github.com/apache/jena/pull/227
For some reason GitHub complains about merge conflicts on this PR, but I
don't see why. The files in question were modified by JENA-1301 (PR #220) but
this branch already contains those commits. I was
40 matches
Mail list logo
