Make some final changes to the GCS NIO API #770

jart · 2016-03-21T20:31:02Z

This change prepares GCS NIO to be merged into master, by making some
last minute API changes.

Documentation has been added to many methods to clarify the preferred
method for instantiating the library, which is the non-SPI version.
Unit tests have been updated to not rely on the SPI, because there's
no way to guarantee clean isolation of SPI usage across tests. We'll
be relying on integration testing to test the SPI interface.
The unit testing methodology has changed somewhat. FakeStorageRpc
should be a private final field on the test class so, if desired,
we'll be able to have the tests dip directly into fake memory.
IOException has been added back to the throws of file system close, in
case we decide to implement the "close all owned channels" thing into
that method in the future.
The getters on the configuration class have been made package-private,
since there's no foreseeable reason they would be needed by the user.
Injectable constructors have been added for Dagger 2 users.

In a future change, a README.md file will be added to replace the
documentation in the package-info.java file.

CC: @jean-philippe-martin @aozarov @cgdecker

aozarov · 2016-03-21T22:11:20Z

Didn't look at the change yet but wanted to ping and mention that the build fails.

...-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageConfiguration.java

   */
-  public abstract int blockSize();
+  public static CloudStorageConfiguration getDefault() {


cgdecker · 2016-03-22T17:26:39Z

Awesome! Exciting to see this becoming open-source.

Btw, shouldn't the files have Apache license headers?

aozarov · 2016-03-22T17:36:39Z

Btw, shouldn't the files have Apache license headers?

Yes, all files should use the same header file that is used for gcloud-java.

/*
 * Copyright 2016 Google Inc. All Rights Reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *       http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

...oud-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystem.java

   */
  @Override
-  public void close() {}
+  public void close() throws IOException {
+    // TODO(jean-philippe-martin,jart): Synchronously close all active channels associated with this


ajkannan · 2016-03-23T00:51:45Z

I took a pass and added a couple nits; the changes looked good to me otherwise. The build issue still needs to be sorted out.

...-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystemProvider.java

-   * Returns Cloud Storage file system, provided a URI with no path, e.g. {@code gs://bucket}.
+   * Returns Cloud Storage file system, provided a URI with no path.
+   *
+   * <p><b>Note:</b> This method should be invoked indirectly via the SPI by calling


This change prepares GCS NIO to be merged into master, by making some last minute API changes. - Documentation has been added to many methods to clarify the preferred method for instantiating the library, which is the non-SPI version. - Unit tests have been updated to not rely on the SPI, because there's no way to guarantee clean isolation of SPI usage across tests. We'll be relying on integration testing to test the SPI interface. - The unit testing methodology has changed somewhat. FakeStorageRpc should be a private final field on the test class so, if desired, we'll be able to have the tests dip directly into fake memory. - IOException has been added back to the throws of file system close, in case we decide to implement the "close all owned channels" thing into that method in the future. - The getters on the configuration class have been made package-private, since there's no foreseeable reason they would be needed by the user. - Injectable constructors have been added for Dagger 2 users. In a future change, a README.md file will be added to replace the documentation in the package-info.java file.

jart · 2016-03-29T20:25:25Z

All comments addressed. PTAL.

jart · 2016-03-30T00:23:05Z

Tests pass.

ajkannan · 2016-03-30T00:26:53Z

Great, I'll take a look at this either today or tomorrow morning.

jart · 2016-03-30T00:33:35Z

Codacy contradicts go/unit-test-practices#naming. Can we disable that check?

ajkannan · 2016-03-30T00:36:36Z

We're looking into getting the method name check disabled on test methods (we've seen this issue a couple times in other PRs too). I believe @mziccard is in contact with folks over at codacy to see how this can get done.

...oud-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystem.java

   *
-   * @see #forBucket(String)
+   * <p><b>Note:</b> It is also possible to instantiate this class via Java's Service Provider
+   * Interface (SPI), e.g. {@code FileSystems.getFileSystem(URI.create("gs://bucket"))}. We


mziccard · 2016-04-22T06:56:30Z

@jart The comment @ajkannan refers to is the one about keeping LocalGcsHelper (that should be renamed to LocalStorageHelper now). Comment is here. I report it here:

Using FakeStorageRpc is fine (and probably a good idea for the reason you state) for the gcs-nio tests, but since the LocalGcsHelper would be helpful for other purposes outside of gcs-nio, I think we should still keep it.
Also, there's a PR in the master branch to rename RemoteGcsHelper to RemoteStorageHelper and LocalGcdHelper to LocalDatastoreHelper; in line with that, we should probably rename LocalGcsHelper to LocalStorageHelper.

I agree with @ajkannan, having a LocalStorageHelper would be very useful for users to test their code without issuing requests to the actual service. In fact, the development of FakeStorageRpc+LocalStorageHelper that you are doing is of huge value to us (set aside the value of the whole gcs-nio).

jart · 2016-04-22T17:59:18Z

First, just to clarify, FakeStorageRpc is @jean-philippe-martin's excellent work, not mine.

LocalGcsHelper is being removed because its design prevents test methods from having access to the fake. It was designed that way because we weren't sure at first how to work around the fact that the service loader stores a permanent instance of the classes we're trying to test. This PR works around that in a different way. A way that's more idiomatic to unit testing. So LocalGcsHelper is no longer necessary.

Also renaming these classes seems unrelated to the work I'm trying to do in this PR. I'm honestly not even sure how I'm going to merge this at the moment, because right now my git client is reporting that this PR has 88 merge conflicts with the gcs-nio branch, due to package renames and removals of static imports.

mziccard · 2016-04-22T18:15:57Z

@jart LocalGcsHelper might not be needed for this PR and for testing gcs-nio but it's still useful for user tests. It's also a pattern we have been adopting through the library so I would ask you too keep it.

Also renaming these classes seems unrelated to the work I'm trying to do in this PR.

I think @ajkannan was just asking to rename LocalGcsHelper to LocalStorageHelper, other renames have already been taken care of.

I'm honestly not even sure how I'm going to merge this at the moment, because right now my git client is reporting that this PR has 88 merge conflicts with the gcs-nio branch, due to package renames and removals of static imports.

Sorry for that. This PR has been open for a while and all kind of refactoring went through in the meantime.

lesv · 2016-05-31T19:23:57Z

Ping - is there anything we can do to for this?

jart · 2016-06-07T20:16:10Z

Due to conflicts, I'm planning on rewriting this PR sometime this week or next week, after we've merged gcs-nio into master.

mziccard · 2016-06-07T20:45:20Z

Due to conflicts, I'm planning on rewriting this PR sometime this week or next week, after we've merged gcs-nio into master.

I believe that before merging gcs-nio into master we will need to fetch master changes into gcs-nio and apply there fixes contained in this PR (or a twin one).

Also other project-wide changes that were made in gcs-nio (e.g. migrating examples to appassembler, other documentation changes) will need to be updated/consolidated before merging into master. For instance we added support for compute and related examples that will have to migrate to app assembler.

jean-philippe-martin · 2016-06-13T16:41:44Z

@mziccard I wouldn't agree that we have to "apply fixes contained in this PR" exactly, since the whole reason we have made zero progress on this since the ancient days of April 1st is the disagreement on LocalGcsHelper: @jart says it is not compatible with the changes in this PR, whereas the commenter (IIUC) argues the PR cannot go in without LocalGcsHelper.

So one option is to merge into master first and then rework the API. The other option would involve having to reach consensus first.

mziccard · 2016-06-13T17:33:50Z

I disagree. This PR contains more fixes than simply the LocalGcsHelper discussion (which BTW lasted 4 messages on the 22nd of April I hardly consider it the reason this PR is blocked). LocalGcsHelper is compatible with changes in this PR, it's just no longer needed for the sake of testing gcs-nio, hence @jart decided to remove it. My suggestion (and @ajkannan's) is to keep it as our users would still benefit from its existence.

I also see that there are a bunch of nio-related issues that we might want to take care of before merging into master.

jart · 2016-06-13T17:42:20Z

I would like to make merging gcs-nio into master my top priority this week. When we originally made this branch, we didn't realize this codebase was already in an alpha state, and being refactored a lot. This has led to problems, such as having to throw away this PR, because it has 70+ merge conflicts.

The shared dependencies BOM provides the latest error_prone_annotations version. No need to specify in individual repositories.

🤖 I have created a release *beep* *boop* --- ### [1.3.1](googleapis/java-resourcemanager@v1.3.0...v1.3.1) (2022-04-27) ### Dependencies * error_prone_annotations from shared deps BOM ([googleapis#770](googleapis/java-resourcemanager#770)) ([13a2edc](googleapis/java-resourcemanager@13a2edc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…r to v2.5.0 (#770) * chore(deps): update dependency com.google.cloud:google-cloud-container to v2.5.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [0.123.3](googleapis/java-datalabeling@v0.123.2...v0.123.3) (2022-08-09) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v3 ([#769](googleapis/java-datalabeling#769)) ([548f6b8](googleapis/java-datalabeling@548f6b8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [2.3.4](googleapis/java-iot@v2.3.3...v2.3.4) (2022-09-15) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.3 ([#769](googleapis/java-iot#769)) ([0470799](googleapis/java-iot@0470799)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [3.0.1](googleapis/java-shared-dependencies@v3.0.0...v3.0.1) (2022-08-02) ### Dependencies * update dependency com.google.code.gson:gson to v2.9.1 ([#766](googleapis/java-shared-dependencies#766)) ([b0d531d](googleapis/java-shared-dependencies@b0d531d)) * update gax.version to v2.18.7 ([#767](googleapis/java-shared-dependencies#767)) ([02e98d5](googleapis/java-shared-dependencies@02e98d5)) * update google.core.version to v2.8.6 ([#770](googleapis/java-shared-dependencies#770)) ([3acea4b](googleapis/java-shared-dependencies@3acea4b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Source-Link: googleapis/googleapis@c2016e5 Source-Link: https://github.com/googleapis/googleapis-gen/commit/0885fd7ae8a805a377974edfba60ab7c9327f75a Copy-Tag: eyJwIjoiamF2YS1jb21wdXRlLy5Pd2xCb3QueWFtbCIsImgiOiIwODg1ZmQ3YWU4YTgwNWEzNzc5NzRlZGZiYTYwYWI3YzkzMjdmNzVhIn0=

…#9230) * feat: Update Compute Engine API to revision 20230307 (#770) Source-Link: googleapis/googleapis@c2016e5 Source-Link: https://github.com/googleapis/googleapis-gen/commit/0885fd7ae8a805a377974edfba60ab7c9327f75a Copy-Tag: eyJwIjoiamF2YS1jb21wdXRlLy5Pd2xCb3QueWFtbCIsImgiOiIwODg1ZmQ3YWU4YTgwNWEzNzc5NzRlZGZiYTYwYWI3YzkzMjdmNzVhIn0= * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 21, 2016

jart force-pushed the nio-api branch from 7428c07 to 9b7d8f9 Compare March 21, 2016 20:31

aozarov added the contrib label Mar 21, 2016

mziccard reviewed Mar 21, 2016
View reviewed changes

...-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageConfiguration.java

*/

public abstract int blockSize();

public static CloudStorageConfiguration getDefault() {

This comment was marked as spam.

Sign in to view

ajkannan reviewed Mar 22, 2016
View reviewed changes

cgdecker reviewed Mar 23, 2016
View reviewed changes

jart force-pushed the nio-api branch from 9b7d8f9 to c0a11cb Compare March 29, 2016 20:25

Address review comments

2ce11cb

jart force-pushed the nio-api branch from c0a11cb to 2ce11cb Compare March 29, 2016 20:53

refactor integration test

af45b49

jart force-pushed the nio-api branch from a432aef to af45b49 Compare March 30, 2016 00:12

aozarov reviewed Mar 30, 2016
View reviewed changes

mziccard mentioned this pull request May 31, 2016

Add ParallelCountBytes #983

Merged

lesv added the in progress label Jun 7, 2016

jart closed this Jun 7, 2016

jart mentioned this pull request Jun 23, 2016

Copy documentation from abandoned PR #1076

Merged

github-actions bot pushed a commit that referenced this pull request Jun 21, 2022

chore: include generated samples dir (#770)

cdebea2

github-actions bot pushed a commit that referenced this pull request Jun 21, 2022

chore: include generated samples dir (#770)

d6713b7

suztomo pushed a commit that referenced this pull request Feb 1, 2023

deps: update google.core.version to v2.8.6 (#770)

3acea4b

release-please bot mentioned this pull request Mar 16, 2023

chore: release main #9225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make some final changes to the GCS NIO API #770

Make some final changes to the GCS NIO API #770

jart commented Mar 21, 2016

aozarov commented Mar 21, 2016

This comment was marked as spam.

cgdecker commented Mar 22, 2016

aozarov commented Mar 22, 2016

This comment was marked as spam.

ajkannan commented Mar 23, 2016

This comment was marked as spam.

jart commented Mar 29, 2016

jart commented Mar 30, 2016

ajkannan commented Mar 30, 2016

jart commented Mar 30, 2016

ajkannan commented Mar 30, 2016

This comment was marked as spam.

mziccard commented Apr 22, 2016

jart commented Apr 22, 2016

mziccard commented Apr 22, 2016

lesv commented May 31, 2016

jart commented Jun 7, 2016

mziccard commented Jun 7, 2016

jean-philippe-martin commented Jun 13, 2016

mziccard commented Jun 13, 2016

jart commented Jun 13, 2016

Make some final changes to the GCS NIO API #770

Make some final changes to the GCS NIO API #770

Conversation

jart commented Mar 21, 2016

aozarov commented Mar 21, 2016

This comment was marked as spam.

cgdecker commented Mar 22, 2016

aozarov commented Mar 22, 2016

This comment was marked as spam.

ajkannan commented Mar 23, 2016

This comment was marked as spam.

jart commented Mar 29, 2016

jart commented Mar 30, 2016

ajkannan commented Mar 30, 2016

jart commented Mar 30, 2016

ajkannan commented Mar 30, 2016

This comment was marked as spam.

mziccard commented Apr 22, 2016

jart commented Apr 22, 2016

mziccard commented Apr 22, 2016

lesv commented May 31, 2016

jart commented Jun 7, 2016

mziccard commented Jun 7, 2016

jean-philippe-martin commented Jun 13, 2016

mziccard commented Jun 13, 2016

jart commented Jun 13, 2016