Skip to content

Conversation

@ehiggs
Copy link

@ehiggs ehiggs commented Jul 10, 2017

Various small fixes to the Hadoop s3guard patch set.

@steveloughran steveloughran force-pushed the s3guard/HADOOP-13786-committer branch from 11c5c4f to 284cd10 Compare July 13, 2017 09:53
@ehiggs ehiggs force-pushed the s3guard/HADOOP-13786-committer branch from 1874cdb to a7d4bfa Compare July 14, 2017 09:19
@ehiggs
Copy link
Author

ehiggs commented Jul 14, 2017

Rebased.

@steveloughran steveloughran force-pushed the s3guard/HADOOP-13786-committer branch from 284cd10 to 5417f2f Compare July 14, 2017 13:24
@steveloughran
Copy link
Owner

heh, I've just gone and broken things by squashing patches

@steveloughran steveloughran force-pushed the s3guard/HADOOP-13786-committer branch from 5417f2f to 284cd10 Compare July 14, 2017 18:25
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't like imports to be moved around. As well as a nominal rule of layout, changes here make merging a nightmare

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. it's "pendingset" lower case.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never knew of this. good to use something else by other people.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring the fact that case-changing classnames is something very unlikely to take reliably in macos unless you can delete the old one first, I'm unsure about this. Why. It's not really a "set" in terms of java lang collection. At the same time, I'm not that fussy about classnames, though I do want it to stay all lower in the filesystem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's two words. Pending and Set. So it's camel cased. Maybe PendingBatch?

The issue about case changing and macos doesn't make sense. The patch wasn't merged yet so no one will have issues with the name change.

@steveloughran
Copy link
Owner

overall

+1 to the new pair class, guava stripping, etc
=0 to the pendingset/pendingSet classchange; -1 to making the filename mixed case
-1 to moving imports around; especially when it doesn't follow the (nominal) hadoop ordering

@ehiggs ehiggs force-pushed the s3guard/HADOOP-13786-committer branch from a7d4bfa to d3119f1 Compare July 15, 2017 20:22
@ehiggs
Copy link
Author

ehiggs commented Jul 15, 2017

I rebased and added a few patched addressing @steveloughran's comments.

I also fixed up some grammar or sentence fragments in the architecture doc.

@steveloughran steveloughran merged this pull request into steveloughran:s3guard/HADOOP-13786-committer Aug 2, 2017
@steveloughran
Copy link
Owner

thanks, merged in

steveloughran added a commit that referenced this pull request Oct 4, 2019
…tify slow points

Listing files is surprisingly slow.

Theories
* the listFiles() call is the wrong scan for local (and HDFS?)
* over use of java 8 streams/maps, etc

explore #2 and then worry about #1. We must stay with listFiles for the
magic committers scans of s3, but for the staging committers, we just need to
flat list the source dir with a filter

Change-Id: I7e29b6004e71b146500a95c9822c5eed17390fb4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants