Add repository-url module and move URLRepository #22752

Tim-Brooks · 2017-01-23T20:57:53Z

This is related to #22116. URLRepository requires SocketPermission
connect. This commit introduces a new module called "repository-url"
where URLRepository will reside. With the new module, permissions can
be removed from core.

…dule

rjernst · 2017-01-23T21:14:27Z

Is there any reason FSRepository would need to be in the same module as URL repository, or could it be separate when it is eventually moved? In which case, this can be called repository-url?

rjernst

This looks good. I left a couple minor comments. The main thing I want to clear up is the name.

rjernst · 2017-01-23T21:16:58Z

core/src/test/java/org/elasticsearch/bwcompat/RestoreBackwardsCompatIT.java

@@ -98,7 +91,8 @@ public void testRestoreOldSnapshots() throws Exception {
        for (Version v : VersionUtils.allReleasedVersions()) {
            if (VersionUtils.isSnapshot(v)) continue;  // snapshots are unreleased, so there is no backcompat yet
            if (v.isRelease() == false) continue; // no guarantees for prereleases
-            if (v.before(Version.CURRENT.minimumIndexCompatibilityVersion())) continue; // we only support versions N and N-1
+            if (v.before(Version.CURRENT.minimumIndexCompatibilityVersion()))


Why this change? If you are going to move the continue to the next line (breaking with the convention of the other loop shortcut statements here...), please open a block.

I'm not sure. I definitely did not intend that change. I'll revert that line.

rjernst · 2017-01-23T21:18:02Z

modules/repositories/build.gradle

+    classname 'org.elasticsearch.plugin.repositories.RepositoriesPlugin'
+}
+
+dependencies {


If there are no dependencies, you don't need a dependencies section.

rjernst · 2017-01-23T21:18:12Z

modules/repositories/build.gradle

+compileJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"
+compileTestJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"
+
+thirdPartyAudit.excludes = [


Not necessary.

rjernst · 2017-01-23T21:18:50Z

...les/repositories/src/main/java/org/elasticsearch/plugin/repositories/RepositoriesPlugin.java

+
+    @Override
+    public Map<String, Repository.Factory> getRepositories(Environment env, NamedXContentRegistry namedXContentRegistry) {
+        return Collections.singletonMap(URLRepository.TYPE, (metadata) -> new URLRepository(metadata, env, namedXContentRegistry));


nit: the parens around metadata are not necessary.

Tim-Brooks · 2017-01-23T21:30:10Z

I'll update the PR with the requested changes.

As for the name - when @s1monw and I originally talked about it, I was going to move both to a module named "repositories". But then this kind of blocked that from being a straightforward change.

So we agreed that while working on the SocketPermissions work I would only move the URLRepository. I kept the naming with the assumption that at some point we would move the FSRepository there also.

But there is no technical reason that I know of why they would need to be in the same module. If we want to specialize this module I can make those changes.

rjernst · 2017-01-23T21:31:46Z

But there is no technical reason that I know of why they would need to be in the same module.

Since URLRepository will require network permission, but FSRepository will not, I think that would be a good split.

jasontedor · 2017-01-23T21:32:29Z

Since URLRepository will require network permission, but FSRepository will not, I think that would be a good split.

+1

Tim-Brooks · 2017-01-24T17:34:30Z

I renamed it to repository-url.

I also worked on fixing some integration tests. I moved a snapshot restore test that used the URLRepository to the new module. That test required a repository path and it looks like you can only call randomRepoPath() on ESIntegTestCase if you're an internal cluster?

In order to force that I a method I had to do this. I'm not sure if this was the correct approach? This was the first time I was dealing with the integration test harness, so let me know if there is something I should be doing instead.

And @s1monw I will need a separate PR to move around connect permissions for all modules/plugins. So I will handle the policy file aspect of this module there.

jasontedor · 2017-01-24T17:36:40Z

I renamed it to url-repository.

I'm really sorry to be a pain here, but I think that it should be repository-url (we have plugins already named repository-*, I think the module should be named consistently).

Tim-Brooks · 2017-01-24T17:37:32Z

@jasontedor I mistyped the comment. repository-url is actually what I named it.

jasontedor · 2017-01-24T17:38:29Z

I mistyped the comment. repository-url is actually what I named it.

Thank you. 😄

rjernst · 2017-01-24T19:01:49Z

@tbrooks8 I don't see where your ignoreExternalCluster() is actually called? But ESIntegTestCase subclasses do not use an external cluster (anymore, they did long ago), so I'm confused on what instigated this added boolean?

Tim-Brooks · 2017-01-24T19:11:47Z

@rjernst Right here.

So when I run gradle :modules:repository-url:integTest without that , this exception is being thrown.

rjernst · 2017-01-24T19:41:12Z

I think you need to rename that test. That exception is there for a reason. It means the test is running with an external cluster (which integTest does everywhere except in core, where the test was pulled from). In core, tests ending in IT extend from ESIntegTestCase. For all other modules, tests ending in IT mean they are running against an external cluster (a true integ test). If you rename the suffix of that test to Tests, it should work like a normal core integ test.

Tim-Brooks · 2017-01-24T22:32:21Z

Thanks. That looks like it was my issue.

…dule

Tim-Brooks · 2017-01-25T17:03:05Z

@jasontedor / @rjernst It looks like the build is passing at this point (now that I have worked out integration test issues). Can you update your reviews and approve it if everything looks alright?

rjernst

LGTM

rjernst · 2017-01-25T19:30:59Z

modules/repository-url/build.gradle

+ */
+
+esplugin {
+    description 'Module for generic repositories'


Should be URL repository?

Yep - just fixed

jasontedor

Looks great, I left a question though.

jasontedor · 2017-01-25T21:51:28Z

modules/repository-url/build.gradle

+}
+
+compileJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"
+compileTestJava.options.compilerArgs << "-Xlint:-unchecked,-rawtypes"


Can you explain why these are needed?

I just copied an existing module and made appropriate modifications when I created this. I assume those are artifacts from that and I do not know any reason those are need. I'll remove them.

Cool, let's indeed see if we can drop them.

jasontedor

LGTM. If the CI build passes, I'm good with it.

Tim-Brooks · 2017-01-25T23:08:43Z

The CI build passed. So I'll merge this.

* master: (47 commits) Remove non needed import use expectThrows instead of manually testing exception Fix checkstyle and a test Update after review Read ec2 discovery address from aws instance tags Invalidate cached query results if query timed out (elastic#22807) Add remaining generated painless API Generate reference links for painless API (elastic#22775) [TEST] Fix ElasticsearchExceptionTests Add parsing method for ElasticsearchException.generateThrowableXContent() (elastic#22783) Improve connection closing in `RemoteClusterConnection` (elastic#22804) Docs: Cluster allocation explain should be on one page Remove DFS_QUERY_AND_FETCH as a search type (elastic#22787) Add repository-url module and move URLRepository (elastic#22752) fix date-processor to a new default year for every new pipeline execution. (elastic#22601) Add tests for top_hits aggregation (elastic#22754) [TEST] Added this for 93a28b0 submitted via elastic#22772 Fix typo in comment in OsProbe.java Add new ruby search library to community clients doc (elastic#22765) RangeQuery WITHIN case now normalises query (elastic#22431) ...

Tim-Brooks added 15 commits January 17, 2017 18:46

Add basic url repository module

ad4f704

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

b32361f

…dule

Fix compile issues

563b616

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

54f6e6b

…dule

Work on restructuring module

311d890

Work on restructuring module

669c4ef

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

00b20f9

…dule

WIP: work on implementing test for urlblobstore

18fefbd

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

8f92848

…dule

Merge branch 'master' into url_repository_module

abd7750

Implement basic blogstore tests

4e05efc

Add URLRepository tests

7bf6f4f

Fix restore bw compat test

b263edc

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

983dc67

…dule

Reorganize packages

6c574e2

Tim-Brooks added :Core/Infra/Core Core issues without another label >enhancement review v6.0.0-alpha1 labels Jan 23, 2017

Tim-Brooks requested review from s1monw and jasontedor January 23, 2017 20:57

rjernst reviewed Jan 23, 2017

View reviewed changes

Tim-Brooks added 4 commits January 23, 2017 15:42

Fix checkstyles and modify for review feedback

6ecf433

Fix line length

ee80d3b

Remove system.out call

ec750a9

Fix forbidden issues

6f661c8

Tim-Brooks added 3 commits January 24, 2017 09:38

Rename url module repository

fa40f2c

Remove dupliated tests

b8b6132

Move integration test

344eeb2

Tim-Brooks changed the title ~~Add repositories module and move URLRepository~~ Add url-repository module and move URLRepository Jan 24, 2017

Tim-Brooks changed the title ~~Add url-repository module and move URLRepository~~ Add repository-url module and move URLRepository Jan 24, 2017

Fix checkstyle errors

937ce94

Tim-Brooks added 2 commits January 24, 2017 15:34

Fix integration test name

63da34d

replace rest test using url repo with fs

b49a346

Tim-Brooks added 4 commits January 24, 2017 17:53

Implement basic yaml integration test

1555ca0

Merge remote-tracking branch 'upstream/master' into url_repository_mo…

e488f29

…dule

Fix integration test by setting config

9ac5852

Use constants

dd03e3a

rjernst approved these changes Jan 25, 2017

View reviewed changes

Fix module description

12775b4

jasontedor reviewed Jan 25, 2017

View reviewed changes

Remove compiler lint args

0880f6c

jasontedor approved these changes Jan 25, 2017

View reviewed changes

Tim-Brooks merged commit 719e75b into elastic:master Jan 25, 2017

Tim-Brooks deleted the url_repository_module branch November 14, 2018 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repository-url module and move URLRepository #22752

Add repository-url module and move URLRepository #22752

Tim-Brooks commented Jan 23, 2017 •

edited

Loading

rjernst commented Jan 23, 2017

rjernst left a comment

rjernst Jan 23, 2017

Tim-Brooks Jan 23, 2017

rjernst Jan 23, 2017

rjernst Jan 23, 2017

rjernst Jan 23, 2017

Tim-Brooks commented Jan 23, 2017

rjernst commented Jan 23, 2017

jasontedor commented Jan 23, 2017

Tim-Brooks commented Jan 24, 2017 •

edited

Loading

jasontedor commented Jan 24, 2017 •

edited

Loading

Tim-Brooks commented Jan 24, 2017

jasontedor commented Jan 24, 2017

rjernst commented Jan 24, 2017 •

edited

Loading

Tim-Brooks commented Jan 24, 2017 •

edited

Loading

rjernst commented Jan 24, 2017

Tim-Brooks commented Jan 24, 2017

Tim-Brooks commented Jan 25, 2017

rjernst left a comment

rjernst Jan 25, 2017

Tim-Brooks Jan 25, 2017

jasontedor left a comment

jasontedor Jan 25, 2017

Tim-Brooks Jan 25, 2017

jasontedor Jan 25, 2017

jasontedor left a comment

Tim-Brooks commented Jan 25, 2017

Add repository-url module and move URLRepository #22752

Add repository-url module and move URLRepository #22752

Conversation

Tim-Brooks commented Jan 23, 2017 • edited Loading

rjernst commented Jan 23, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks commented Jan 23, 2017

rjernst commented Jan 23, 2017

jasontedor commented Jan 23, 2017

Tim-Brooks commented Jan 24, 2017 • edited Loading

jasontedor commented Jan 24, 2017 • edited Loading

Tim-Brooks commented Jan 24, 2017

jasontedor commented Jan 24, 2017

rjernst commented Jan 24, 2017 • edited Loading

Tim-Brooks commented Jan 24, 2017 • edited Loading

rjernst commented Jan 24, 2017

Tim-Brooks commented Jan 24, 2017

Tim-Brooks commented Jan 25, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Tim-Brooks commented Jan 25, 2017

Tim-Brooks commented Jan 23, 2017 •

edited

Loading

Tim-Brooks commented Jan 24, 2017 •

edited

Loading

jasontedor commented Jan 24, 2017 •

edited

Loading

rjernst commented Jan 24, 2017 •

edited

Loading

Tim-Brooks commented Jan 24, 2017 •

edited

Loading