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

Add Term Vectors API for Elasticsearch 1.x (elastic.v2) #179

Closed
wants to merge 7 commits into
base: release-branch.v2
from

Conversation

Projects
None yet
2 participants
@t2y

t2y commented Dec 2, 2015

I found elastic doesn't support Term Vector API now. So, I have made the mappings to call the termvector API. This is my first contribution to elastic. It might not be satisfied with contributing guidelines. If so, tell me how to do completely and I'll fix it.

Term Vector API has improved Elasticsearch 2.x. For example, it renamed termvector to termvectors . This PR is for Elasticsearch 1.x.

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 2, 2015

Owner

@t2y Thanks a lot. This is looking really good.

Just a few words regarding the implementation of services. First, I try to use the Java client as the root source rather than the documentation. The official documentation often lacks details where the Java source even shows off the generated JSON structure for every edge case. I also use the Java source for creating the responses. E.g. here's the code for TermVectorRequestBuilder and TermVectorResponse for 1.7. Maybe you're missing some information in the response (didn't look it up).

Second, did you see that the generate-api branch has a Go generator for services. It takes the REST specification and generates the Go services. Well, it doesn't do it right 100% of the time (it doesn't get the GET/POST/PUT right sometimes), but at least it pumps out all properties available according to the REST specification. So if there's a service missing, I try to use the Go generator to create a 90% template for me.

Again, thanks for the PR. I will try to merge soon.

Owner

olivere commented Dec 2, 2015

@t2y Thanks a lot. This is looking really good.

Just a few words regarding the implementation of services. First, I try to use the Java client as the root source rather than the documentation. The official documentation often lacks details where the Java source even shows off the generated JSON structure for every edge case. I also use the Java source for creating the responses. E.g. here's the code for TermVectorRequestBuilder and TermVectorResponse for 1.7. Maybe you're missing some information in the response (didn't look it up).

Second, did you see that the generate-api branch has a Go generator for services. It takes the REST specification and generates the Go services. Well, it doesn't do it right 100% of the time (it doesn't get the GET/POST/PUT right sometimes), but at least it pumps out all properties available according to the REST specification. So if there's a service missing, I try to use the Go generator to create a 90% template for me.

Again, thanks for the PR. I will try to merge soon.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 2, 2015

Thank you for detailed reply.

I also use the Java source for creating the responses. (snip) Maybe you're missing some information in the response (didn't look it up).

Oh, that's a good method. I'll check and update seeing the Java source.

did you see that the generate-api branch has a Go generator for services. It takes the REST specification and generates the Go services.

Yes. I originally made the source file from the generator, then I updated a few lines and the response object mappings. That's a notable tool. I learnt Go's potential using it.

t2y commented Dec 2, 2015

Thank you for detailed reply.

I also use the Java source for creating the responses. (snip) Maybe you're missing some information in the response (didn't look it up).

Oh, that's a good method. I'll check and update seeing the Java source.

did you see that the generate-api branch has a Go generator for services. It takes the REST specification and generates the Go services.

Yes. I originally made the source file from the generator, then I updated a few lines and the response object mappings. That's a notable tool. I learnt Go's potential using it.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 2, 2015

@olivere

I implemented some fields in the response object and some parameters for API referring Java source. Is there any task?

t2y commented Dec 2, 2015

@olivere

I implemented some fields in the response object and some parameters for API referring Java source. Is there any task?

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 2, 2015

Owner

@t2y Seems okay. Only thing I'd change is to use map[string]string instead of *map[string]string and then test for len(doc) > 0 instead of doc != nil.

Owner

olivere commented Dec 2, 2015

@t2y Seems okay. Only thing I'd change is to use map[string]string instead of *map[string]string and then test for len(doc) > 0 instead of doc != nil.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 2, 2015

@olivere Thank you for reviewing. I fixed it.

t2y commented Dec 2, 2015

@olivere Thank you for reviewing. I fixed it.

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

@t2y Is it a good idea to add the id parameter here. In contrast to index and typ it is optional according to the REST spec. Can we agree to have only index and typ here. This would change the TV code from

client.TermVector("twitter", "tweet", "1")

to

client.TermVector("twitter", "tweet").Id("1")

olivere commented on client.go in fd75d19 Dec 3, 2015

@t2y Is it a good idea to add the id parameter here. In contrast to index and typ it is optional according to the REST spec. Can we agree to have only index and typ here. This would change the TV code from

client.TermVector("twitter", "tweet", "1")

to

client.TermVector("twitter", "tweet").Id("1")

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Owner

OK. I fixed it.

Owner

t2y replied Dec 3, 2015

OK. I fixed it.

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

@t2y Can you also fix the documentation here as well as in the other places? It should read as an English sentence.

olivere commented on termvector.go in fd75d19 Dec 3, 2015

@t2y Can you also fix the documentation here as well as in the other places? It should read as an English sentence.

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Owner

I just replaced .. to ., is it make sense?

Owner

t2y replied Dec 3, 2015

I just replaced .. to ., is it make sense?

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

:-) No. I guess I wasn't clear. I also wanted to get rid of the "... is documented as: ..." everywhere. Don't worry, I can also fix this before I review and merge the PR.

olivere replied Dec 3, 2015

:-) No. I guess I wasn't clear. I also wanted to get rid of the "... is documented as: ..." everywhere. Don't worry, I can also fix this before I review and merge the PR.

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

If you still want to work on it, also make sure the godoc wants the name of the func as the first word in the documentation header. E.g. if the function is called Index, then the documentation should start with Index. See here.

olivere replied Dec 3, 2015

If you still want to work on it, also make sure the godoc wants the name of the func as the first word in the documentation header. E.g. if the function is called Index, then the documentation should start with Index. See here.

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Owner

Thanks for good information. I haven't used godoc by myself yet, so I would like you to fix the documentation. After that, I will learn how to do it. 😄

Owner

t2y replied Dec 3, 2015

Thanks for good information. I haven't used godoc by myself yet, so I would like you to fix the documentation. After that, I will learn how to do it. 😄

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

Okay. Will do. No worries.

olivere replied Dec 3, 2015

Okay. Will do. No worries.

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

@t2y Watch out: This does not work if id is missing! TV has two endpoints. One with index and type, and another with index, type, and id. Can you please also add a test for buildURL and the two endpoints (see other tests)?

olivere commented on termvector.go in fd75d19 Dec 3, 2015

@t2y Watch out: This does not work if id is missing! TV has two endpoints. One with index and type, and another with index, type, and id. Can you please also add a test for buildURL and the two endpoints (see other tests)?

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Owner

OK. buildURL test was added.

Owner

t2y replied Dec 3, 2015

OK. buildURL test was added.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Can you also fix the documentation here as well as in the other places? It should read as an English sentence.

@olivere Which document should I fix? I'm not good at English writing, but I'll try if it's boilerplate task.

t2y commented Dec 3, 2015

Can you also fix the documentation here as well as in the other places? It should read as an English sentence.

@olivere Which document should I fix? I'm not good at English writing, but I'll try if it's boilerplate task.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

Can you please also add a test for buildURL and the two endpoints (see other tests)?

Thanks. I'll write a test referring other test cases.

t2y commented Dec 3, 2015

Can you please also add a test for buildURL and the two endpoints (see other tests)?

Thanks. I'll write a test referring other test cases.

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

Owner

@t2y Open this link. My comments are inlined so you see the exact location where I added notes.

Re: documentation. It's just a formatting issue, e.g. instead of Index is documented as: The index in which the document resides.. it should read as Index in which the document resides.

Owner

olivere commented Dec 3, 2015

@t2y Open this link. My comments are inlined so you see the exact location where I added notes.

Re: documentation. It's just a formatting issue, e.g. instead of Index is documented as: The index in which the document resides.. it should read as Index in which the document resides.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

@olivere I got an error on Travis CI, but this error doesn't happen on my local environment. Do you know how to fix?

--- FAIL: TestTermVectorWithDoc (0.07s)
termvector_test.go:70: elastic: Error 503 (Service Unavailable):
  NoShardAvailableActionException[[elastic-test][0] null]; nested:
  IllegalIndexShardStateException[[elastic-test][0] CurrentState[POST_RECOVERY]
  operations only allowed when started/relocated]; 

t2y commented Dec 3, 2015

@olivere I got an error on Travis CI, but this error doesn't happen on my local environment. Do you know how to fix?

--- FAIL: TestTermVectorWithDoc (0.07s)
termvector_test.go:70: elastic: Error 503 (Service Unavailable):
  NoShardAvailableActionException[[elastic-test][0] null]; nested:
  IllegalIndexShardStateException[[elastic-test][0] CurrentState[POST_RECOVERY]
  operations only allowed when started/relocated]; 
@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 3, 2015

Owner

@t2y Travis is wonky sometimes. Often a small sleep interval helps, sometimes manually refreshing or flushing after indexing data. Sometimes you can even restart the test and it works on the 2nd run. Don't worry about ES errors too much. Non-ES errors are more important. I will run tests again when merging the PR. If I can't get it to run reliably on Travis, I'm even considering to disable it on CI.

Owner

olivere commented Dec 3, 2015

@t2y Travis is wonky sometimes. Often a small sleep interval helps, sometimes manually refreshing or flushing after indexing data. Sometimes you can even restart the test and it works on the 2nd run. Don't worry about ES errors too much. Non-ES errors are more important. I will run tests again when merging the PR. If I can't get it to run reliably on Travis, I'm even considering to disable it on CI.

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 3, 2015

@olivere I see. Now, I don't fix the test code. Please add sleep interval or disable it on CI when you think so.

t2y commented Dec 3, 2015

@olivere I see. Now, I don't fix the test code. Please add sleep interval or disable it on CI when you think so.

olivere added a commit that referenced this pull request Dec 4, 2015

@olivere

This comment has been minimized.

Show comment
Hide comment
@olivere

olivere Dec 4, 2015

Owner

Merged and should be available in 2.0.23. Thanks for your support.

Owner

olivere commented Dec 4, 2015

Merged and should be available in 2.0.23. Thanks for your support.

@olivere olivere closed this Dec 4, 2015

@t2y

This comment has been minimized.

Show comment
Hide comment
@t2y

t2y Dec 4, 2015

Thanks a lot! 😄

t2y commented Dec 4, 2015

Thanks a lot! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment