Reorg and refactor repo - continued #1684

chanseokoh · 2019-05-01T23:08:29Z

For Blob changes, following the same spirit in #1647 and #1678.

Other changes involve removing various classes around progress reporting, decoupling functionalities to have small, fine-grained general components, etc.

chanseokoh · 2019-05-07T15:23:39Z

jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java

@@ -27,10 +29,37 @@
 */
 public class Authorization {

+  /**


note to reviewers: these are just moved from Authorizations.

jib-core/src/main/java/com/google/cloud/tools/jib/event/progress/DelayedConsumer.java

chanseokoh · 2019-05-07T15:46:07Z

Ready for review.

TadCordle · 2019-05-07T20:22:47Z

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryClient.java

-   * @param blobProgressListener listener for progress of the pull
+   * @param blobSizeListener callback to receive the total size of the BLOb to pull
+   * @param writtenByteCountListener listens on byte count written to an output stream during the
+   *     pull
   * @return a {@link Blob}
   */
  public Blob pullBlob(


Maybe it could be simpler if these blob pull/push clsases took a single ProgressEventDispatcherWrapper with the set/dispatch methods instead of two consumers? I'm not sure how it fits into the class hierarchy though.

That's true it will look simpler technically. But my intent is actually decouple ProgressEventDispatcherWrapper from RegistryClient, which I think makes sense from the API perspective because this ProgressEventDispatcherWrapper is too specific in that it is an unfortunate wrapper required to lazily instantiate ProgressEventDispatcher. I think this class can remain general and free of progress event dispatcher stuff, especially the wrapper. That way I specifically named them writtenByCountListener instead of, say, progressEventDispatcher.

TadCordle · 2019-05-07T20:27:19Z

...e/src/main/java/com/google/cloud/tools/jib/builder/steps/ProgressEventDispatcherWrapper.java

@@ -65,8 +57,9 @@ public void close() {
    progressEventDispatcher.close();
  }

-  void initializeWithBlobSize(long blobSize) {
+  void setProgressTarget(long allocationUnits) {


Seems a bit weird to have to call the constructor and this method to fully initialize a ProgressEventDispatcherWrapper, maybe we can consolidate this by changing the signature of RegistryClient#pullBlob?

Yeah, I did not like this wrapper either. The problem Q originally faced was the progress reporting design that

You have to know and set in advance how much progress units you will allocate when creating a progress event dispatcher.

But you cannot know how much bytes (allocation units) you will download until you read Content-Length.

So the wrapper that will create and hold a progress event dispatcher when this method is called. Other than that, this class is just a thin wrapper that delegates dispatchProgress().

For this reason, obviously it's best to get rid of this unfortunate wrapper if possible. As an attempt to work around the issue, Q probably also felt it doesn't make much sense to have this wrapper in the signature and make it a hard dependency from the API standpoint, also given that there are only just a couple places this wrapper is required. And as I said in the earlier comment, my intention is to actually decouple progress dispatcher stuff from RegistryClient.

briandealwis

Some comments from the half-way mark.

jib-core/src/main/java/com/google/cloud/tools/jib/event/progress/DelayedConsumer.java

jib-core/src/main/java/com/google/cloud/tools/jib/hash/CountingDigestOutputStream.java

briandealwis · 2019-05-10T20:46:18Z

jib-core/src/main/java/com/google/cloud/tools/jib/hash/CountingDigestOutputStream.java

-   * resets after this method is called, so this method should only be called once per computation.
+   * Computes the hash and returns it along with the size of the bytes written to compute the hash.
+   * The buffer resets after this method is called, so this method should only be called once per
+   * computation.


I wonder how many places do we call this method multiple times?

I'm guessing none, because the last time I touched this class, it was reporting wrong bytes when called more than once. (Basically, bytesSoFar was not reset.)

jib-core/src/main/java/com/google/cloud/tools/jib/http/BlobHttpContent.java

chanseokoh · 2019-05-14T21:34:52Z

I've addressed the comments. This should be good now.

jib-core/src/main/java/com/google/cloud/tools/jib/event/progress/ThrottledConsumer.java

jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java

jib-core/src/main/java/com/google/cloud/tools/jib/event/progress/ThrottledLongConsumer.java

jib-core/src/main/java/com/google/cloud/tools/jib/hash/Digests.java

jib-core/src/main/java/com/google/cloud/tools/jib/http/ListenableCountingOutputStream.java

loosebazooka

looks good to me, unless @briandealwis has any further questions?

… into cleanup-4

briandealwis · 2019-05-17T14:50:20Z

jib-core/src/main/java/com/google/cloud/tools/jib/http/BlobHttpContent.java

+    this(blob, contentType, ignored -> {});
+  }
+
+  public BlobHttpContent(Blob blob, String contentType, Consumer<Long> writtenByteCountListener) {


It seems weird that a content object carries a listener — it's entirely possible this content object could be written out multiple times.

Makes sense. Not immediately clear how I can change this though.

Should it not contain a writeTo? Or should writeTo take the listener?

The class is implementing HttpContent::writeTo, so it's not like we can simply change the signature at this level. Perhaps next time.

Ah interesting, didn't know HttpContent was something outside of our codebase.

chanseokoh added 28 commits April 19, 2019 11:35

Decouple Blob

0688f28

Rename ImageToTarTranslator

ec340f3

Rename test class

305f254

Decouple Blob from TarStreamBuilder

02b42ad

empty commit to stalled Kokoro

0ff040a

Clean up

ac3ef64

more intuitive code

447573d

wip

8098dc3

wip

181dbad

Merge branch 'master' into cleanup-3

ca070aa

Revert TarStreamBuilder

efd85ef

wip

21067f7

Merge branch 'master' into cleanup-3

96a047e

Remove Blob dep from DigestUtil

ec4a67f

Move WritableContents to hash package

2cf3c4b

More refactoring

e5bb5e9

Decouple Blob from http Response

8b82327

wip

873ada8

Decouple Blob from http Request

a647b80

test

8f96337

Reorg and refactor

d3bfc7a

Merge branch 'master' into cleanup-2

c047f8c

wip

11d7b60

done

52c6455

Fix wrong filename

f293268

Remove more Blob indirection

e4461b4

Merge branch 'cleanup-2' into cleanup-4

c7aee9e

Rename

40f6666

googlebot added the cla: yes label May 1, 2019

Rename

d6f02da

chanseokoh added 5 commits May 7, 2019 11:00

Clean up

f2be62e

Fix Javadocs

e75f8e0

Update Javadocs

523b4f7

Merge remote-tracking branch 'origin/master' into cleanup-4

a746d54

Rename

c74330c

chanseokoh commented May 7, 2019

View reviewed changes

chanseokoh marked this pull request as ready for review May 7, 2019 15:41

chanseokoh changed the title ~~Draft - reorg and refactor part 2~~ Reorg and refactor repo - continued May 7, 2019

TadCordle reviewed May 7, 2019

View reviewed changes

Merge branch 'master' into cleanup-4

e2b3434

chanseokoh requested a review from a team May 10, 2019 17:54

briandealwis reviewed May 10, 2019

View reviewed changes

Push up throttling decision

b8640af

loosebazooka reviewed May 15, 2019

View reviewed changes

jib-core/src/main/java/com/google/cloud/tools/jib/event/progress/ThrottledConsumer.java Outdated Show resolved Hide resolved

jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java Outdated Show resolved Hide resolved

chanseokoh added 3 commits May 15, 2019 14:10

Rename "with..." to "from..."

3fba22b

Specialize ThrottledConsumer

6f9ea9b

Fix Javadoc

70d0747

briandealwis reviewed May 16, 2019

View reviewed changes

chanseokoh added 4 commits May 16, 2019 00:18

ListenableCountingOutputStream -> NotifyingOutputStream

998d56a

optionalOutStream -> outStream

70be4b2

ThrottledLongConsumer -> ThrottledAccumulatingConsumer

d6842e6

ThrottledLongConsumer -> ThrottledAccumulatingConsumer

c5fedc7

loosebazooka approved these changes May 16, 2019

View reviewed changes

chanseokoh added 2 commits May 17, 2019 11:02

Merge branch 'master' into cleanup-4

60778ee

Merge branch 'cleanup-4' of https://github.com/GoogleContainerTools/jib…

421a032

… into cleanup-4

briandealwis approved these changes May 17, 2019

View reviewed changes

chanseokoh merged commit ccfa35e into master May 17, 2019

chanseokoh deleted the cleanup-4 branch May 17, 2019 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorg and refactor repo - continued #1684

Reorg and refactor repo - continued #1684

chanseokoh commented May 1, 2019 •

edited

Loading

chanseokoh May 7, 2019

chanseokoh commented May 7, 2019

TadCordle May 7, 2019

chanseokoh May 7, 2019

TadCordle May 7, 2019

chanseokoh May 7, 2019

briandealwis left a comment

briandealwis May 10, 2019

chanseokoh May 10, 2019

chanseokoh commented May 14, 2019

loosebazooka left a comment •

edited

Loading

briandealwis May 17, 2019

chanseokoh May 17, 2019

loosebazooka May 17, 2019

chanseokoh May 17, 2019

loosebazooka May 17, 2019

Reorg and refactor repo - continued #1684

Reorg and refactor repo - continued #1684

Conversation

chanseokoh commented May 1, 2019 • edited Loading

Choose a reason for hiding this comment

chanseokoh commented May 7, 2019

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

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented May 14, 2019

loosebazooka left a comment • edited Loading

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

chanseokoh commented May 1, 2019 •

edited

Loading

loosebazooka left a comment •

edited

Loading