filesystem: move file object with stats, threading + flushing to AccessLogFile; add generic File object by sesmith177 · Pull Request #5772 · envoyproxy/envoy

sesmith177 · 2019-01-30T17:31:08Z

Description:
The RawFile + RawInstance classes will be used to hold platform-specific filesystem code. The File/Instance classes will be used to add stats collecting and periodic flushing -- they will be platform agnostic.

The old Filesystem::File object (with stats, threading, etc.) is only used by the AccessLogManager, so move it into that interface. It is now completely cross-platform. Replace Filesystem::File with a basic file object that each platform (Windows, Linux, etc..) can use to provide its own implementation

This is step 2/3 in adding Windows filesystem support as outlined here: #5470 (comment).

Risk Level:
Low

Testing:
bazel build //source/... && bazel test //test/...

Docs Changes:
N/A
Release Notes:
N/A

The RawFile + RawInstance classes will be used to hold platform-specific filesystem code. The File/Instance classes will be used to add stats collecting and periodic flushing -- they will be platform agnostic. This is step 2/3 in adding Windows filesystem support. Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Sophie Wigmore <swigmore@pivotal.io>

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz

Nice -- just a few nits. Apologies for the delay.

jmarantz · 2019-02-05T19:25:54Z

source/common/filesystem/filesystem_impl.cc

-      os_sys_calls_.open(path_, O_RDWR | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
-  fd_ = result.rc_;
-  if (-1 == fd_) {
+  const auto result = raw_file_->open();


Explicit type here as it's not obvious from context.

https://google.github.io/styleguide/cppguide.html#auto

jmarantz · 2019-02-05T19:27:07Z

source/common/filesystem/filesystem_impl.cc

-    os_sys_calls_.close(fd_);
+    const auto result = raw_file_->close();
+    ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", raw_file_->path(),
+                                   strerror(result.errno_)));


IMO we should be abstracting this need to do strerror() in the RawFile interface.

jmarantz · 2019-02-05T19:29:13Z

source/common/filesystem/raw_instance_impl.cc

+RawFileImpl::RawFileImpl(const std::string& path) : fd_(-1), path_(path) {}
+
+RawFileImpl::~RawFileImpl() {
+  const auto result = close();


explicit type please.

jmarantz · 2019-02-05T19:31:21Z

source/common/filesystem/raw_instance_impl.cc

+  // TODO(htuch): Optimize this as a hash lookup if we grow any further.
+  if (absl::StartsWith(canonical_path.rc_, "/dev") ||
+      absl::StartsWith(canonical_path.rc_, "/sys") ||
+      absl::StartsWith(canonical_path.rc_, "/proc")) {


I think someone else has an outstanding PR that adds to this list. Can you merge master?

jmarantz · 2019-02-05T19:33:28Z

test/common/filesystem/raw_instance_impl_test.cc

+  ::unlink(new_file_path.c_str());
+
+  RawFilePtr file = raw_instance_.createRawFile(new_file_path);
+  const auto result = file->open();


explicit type

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

sesmith177 · 2019-02-05T20:32:56Z

@jmarantz thanks, we've addressed your feedback

jmarantz · 2019-02-05T20:49:53Z

source/common/filesystem/filesystem_impl.cc

+          const Api::SysCallBoolResult result = raw_file_->close();
          ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", raw_file_->path(),
-                                         strerror(result.errno_)));
+                                         raw_file_->errorToString(result.errno_)));


Better, but I feel like even the errno_ could be abstracted here. Can we have errorString() be a method on Api::SysCallBoolResult, or something like that?

E.g. I was envisioning on Windows or some other hypothetical OS that an error might have a different representation than an 'int' for errors. It might be an enum or some other abstract type. WDYT?

We could also do this on a follow-up.

I think for now we can hold off on addressing this, since our implementation of RawFile on Windows will also use errno + strerror as well.

This will definitely come up when we start PRing socket changes upstream (e.g. an implementation of #5829). In Windows sockets, errors are not reported in errno and are not displayed using strerror. With the full context, I think will be able to come up with the correct abstraction

OK can you add a TODO for that?

Thanks!

we can add the TODO

jmarantz · 2019-02-05T21:18:40Z

source/common/filesystem/filesystem_impl.cc

+          const Api::SysCallBoolResult result = raw_file_->close();
          ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", raw_file_->path(),
-                                         strerror(result.errno_)));
+                                         raw_file_->errorToString(result.errno_)));


OK can you add a TODO for that?

Thanks!

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

mattklein123

Thanks a ton for continuing to slog through this. I have some interface questions to start out with.

/wait

mattklein123 · 2019-02-06T01:13:58Z

include/envoy/filesystem/filesystem.h

+ * Abstraction for a basic file on disk.
 */
-class File {
+class RawFile {


From an interface perspective, I'm not really sure what the difference is between a "raw" file and a "file." Can you at minimum add more comments and maybe think of a more descriptive name for File below?

+1 for more comments. From my perspective a RawFile is an OS abstraction, and a File (maybe a better name would be good) adds Envoy semantics like stats and auto-flush.

RawFile needs to be re-implemented for different physical file layers, but the stats & auto-flush functionality can be shared across multiple RawFile impls.

As far as I can tell, the only use of the File class is by the AccessLogManager, so maybe AccessLogFile or something like that might be better?

If we leave it here I would probably do something like ThreadSafeFile or ThreadSafeAutoFlushFile or something. One other option is to actually completely move this file type and its creation into the access log manager interface, and then the filesystem interface just knows how to make "raw" or basic files, and then we can just call it File. WDYT?

I think moving it into the access log manager interface makes sense -- we'll give it a look

mattklein123 · 2019-02-06T01:14:52Z

include/envoy/filesystem/filesystem.h

+
+typedef std::unique_ptr<RawFile> RawFilePtr;
+
+class RawInstance {


It seems a little strange to me that we have a "raw" filesystem instance and then a "normal" filesystem instance. Why not just have a single filesystem that can create either "raw" or "regular" files? (Final names TBD).

The issue is that creating the "regular" file requires a Dispatcher, a lock, stats, and threading. In the final version of this (with Windows support), we want to be able to inject the platform specific filesystem code from here: https://github.com/envoyproxy/envoy/blob/f9107b26ccca409a13716b7b094bd87fec9765fb/source/exe/win32/platform_impl.h, similarly to how we inject the platform specific threading code.

To get around this, the "raw" filesystem instance doesn't know how to create "regular" files -- it is injected to the "normal" instance that does.

If this too strange, then maybe just the Api object will be able to create "regular" files (since they are not platform specific). This would end up reverting a bit of #5692, which wouldn't be a problem, except for the small bit of churn

See my comment above. Should we just have "regular" files be part of the access log manager interface since this is the interface the reopens them, etc.?

jmarantz · 2019-02-06T02:20:08Z

include/envoy/filesystem/filesystem.h

+ * Abstraction for a basic file on disk.
 */
-class File {
+class RawFile {


+1 for more comments. From my perspective a RawFile is an OS abstraction, and a File (maybe a better name would be good) adds Envoy semantics like stats and auto-flush.

RawFile needs to be re-implemented for different physical file layers, but the stats & auto-flush functionality can be shared across multiple RawFile impls.

jmarantz · 2019-02-06T02:20:31Z

include/envoy/filesystem/filesystem.h

+  virtual std::string errorToString(int error) PURE;
+};
+
+typedef std::unique_ptr<RawFile> RawFilePtr;


prefer using RawFilePtr = std::unique_ptr<RawFile>;

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Sam Smith <sesmith177@gmail.com> wip Signed-off-by: Sam Smith <sesmith177@gmail.com> wip Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-07T19:17:31Z

@mattklein123 we've moved the old Filesystem::File into AccessLog::AccessLogFile and updated the Api object accordingly

mattklein123 · 2019-02-09T21:27:03Z

From an interface perspective this looks awesome. +1. Looks like needs a master merge. @jmarantz do you mind taking a pass over the reworked code?

/wait

jmarantz · 2019-02-10T18:58:28Z

include/envoy/filesystem/filesystem.h

+
+  /**
+   * @return string a human-readable string describing the error code
+   * TODO(sesmith177) Abstract this method so it isn't dependant on integer error codes


Can you explicitly reference IOError in pending #5829 , ( https://github.com/envoyproxy/envoy/pull/5829/files#diff-8a502ef7d52f5c01c9dc7240386e9fb4R20 )

I think that's what we'll want to use for this. Not sure if this will merge first but we should definitely converge them.

Sure, I'll update the TODO

jmarantz · 2019-02-10T19:09:49Z

test/test_common/utility.cc

 class TestImpl : public TimeSystemProvider, public Impl {
 public:
-  TestImpl(std::chrono::milliseconds file_flush_interval_msec,
-           Thread::ThreadFactory& thread_factory, Stats::Store& stats_store)


That stats_store was previously needed for the stats kept by the file-system. Do we not need that anymore?

See also #5900 which eliminates that arg when the store is not needed.

The API doesn't need the stats_store anymore since it was only ever used by the Filesystem::File object, which is now in AccessLog::AccessLogFile

I see...that's now in AccessLogManager. Don't we want the API to provide the AccessLogManager though, and thus still need access to the StatsStore?

Answering myself: the AccessLogManager was previously existing and is owned by the server, which has access to the StatsStore, so it's not needed for that.

I also have to admit that I was planning (but did not document) to more broadly require access to StatsStore especially at initialization time, as part of my long-ongoing #4980 . So if you remove StatsStore from a bunch of call-sites in this PR I'm going to need to re-add it later. I'm OK with that, but I guess I'd say at the risk of speculative generality that providing access to the StatsStore is a reasonable core competency for the API object :)

I agree that access to the StatsStore is reasonable functionality for the API object. While it feels a little odd to pass in an object to the API that won't be used, I think it's probably OK if we expect it to be used in the future

We added back the StatsStore argument to the Api constructor -- it's just ignored for now

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz

I started enumerating some eliminations of the stats_ arg to revert, but then realized you could just as easily go through this process yourself :)

Just search in your Files tab in the PR all the instances to createApiForTest and revert to prior state.

Thanks :)

jmarantz · 2019-02-13T14:35:04Z

test/common/runtime/runtime_impl_test.cc

 class DiskBackedLoaderImplTest : public TestBase {
 protected:
-  DiskBackedLoaderImplTest() : api_(Api::createApiForTest(store)) {}
+  DiskBackedLoaderImplTest() : api_(Api::createApiForTest()) {}


jmarantz · 2019-02-13T14:35:13Z

test/common/stats/thread_local_store_speed_test.cc

 public:
  ThreadLocalStorePerf()
-      : store_(options_, heap_alloc_), api_(Api::createApiForTest(store_, time_system_)) {
+      : store_(options_, heap_alloc_), api_(Api::createApiForTest(time_system_)) {


jmarantz · 2019-02-13T14:35:23Z

test/common/upstream/cds_api_impl_test.cc

 class CdsApiImplTest : public TestBase {
 protected:
-  CdsApiImplTest() : request_(&cm_.async_client_), api_(Api::createApiForTest(store_)) {}
+  CdsApiImplTest() : request_(&cm_.async_client_), api_(Api::createApiForTest()) {}


jmarantz · 2019-02-13T14:35:29Z

test/common/upstream/cluster_manager_impl_test.cc

 class TestClusterManagerFactory : public ClusterManagerFactory {
 public:
-  TestClusterManagerFactory() : api_(Api::createApiForTest(stats_)) {
+  TestClusterManagerFactory() : api_(Api::createApiForTest()) {


jmarantz · 2019-02-13T14:35:37Z

test/common/upstream/eds_test.cc

 class EdsTest : public TestBase {
 protected:
-  EdsTest() : api_(Api::createApiForTest(stats_)) { resetCluster(); }
+  EdsTest() : api_(Api::createApiForTest()) { resetCluster(); }


jmarantz · 2019-02-13T14:35:42Z

test/common/upstream/hds_test.cc

  HdsTest()
      : retry_timer_(new Event::MockTimer()), server_response_timer_(new Event::MockTimer()),
-        async_client_(new Grpc::MockAsyncClient()), api_(Api::createApiForTest(stats_store_)),
+        async_client_(new Grpc::MockAsyncClient()), api_(Api::createApiForTest()),


jmarantz · 2019-02-13T14:35:50Z

test/common/upstream/logical_dns_cluster_test.cc

 class LogicalDnsClusterTest : public TestBase {
 protected:
-  LogicalDnsClusterTest() : api_(Api::createApiForTest(stats_store_)) {}
+  LogicalDnsClusterTest() : api_(Api::createApiForTest()) {}


jmarantz · 2019-02-13T14:35:59Z

test/common/upstream/original_dst_cluster_test.cc

  OriginalDstClusterTest()
-      : cleanup_timer_(new Event::MockTimer(&dispatcher_)),
-        api_(Api::createApiForTest(stats_store_)) {}
+      : cleanup_timer_(new Event::MockTimer(&dispatcher_)), api_(Api::createApiForTest()) {}


Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-13T15:52:57Z

@jmarantz reverted the createApiForTest changes

sesmith177 · 2019-02-13T17:33:01Z

/retest

repokitteh-read-only · 2019-02-13T17:33:08Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #5772 (comment) was created by @sesmith177.

see: more, trace.

jmarantz

thanks for doing this; I'm good to go modulo the changes I suggested, but this definitely requires senior maintainer review.

jmarantz · 2019-02-15T22:57:19Z

source/common/filesystem/filesystem_impl.cc

+
+Api::SysCallBoolResult FileImpl::close() {
+  if (!isOpen()) {
+    return {true, 0};


The doc in the interface is ambiguous, but this allows sloppy closes. WDYT of considering it an ASSERT() failure to close a non-open file, and in production to at least return false.

You'd have to change the destructor to close only if open of course.

Note the underlying system call at least returns an error if you close a file that isn't open. I couldn't tell on a quick scan if this would be incompatible with current usage in Envoy though. In which case a comment as to why you are going down this sloppy path is needed.

jmarantz · 2019-02-15T23:00:31Z

test/common/access_log/access_log_manager_impl_test.cc

+}
+
+TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) {
+  NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);


consider using SimulatedTimeSystem rather than mocking timers. It might be a cleaner test. It didn't look like they were carried over from master.

These tests were carried over from master, e.g.:

envoy/test/common/filesystem/filesystem_impl_test.cc

Line 127 in 6c95ed9

TEST_F(FileSystemImplTest, flushToLogFilePeriodically) {

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-21T13:53:02Z

@jmarantz @mattklein123 addressed feedback

sesmith177 · 2019-02-21T15:20:36Z

/retest

repokitteh-read-only · 2019-02-21T15:20:41Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #5772 (comment) was created by @sesmith177.

see: more, trace.

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123

Awesome!

mattklein123 · 2019-02-21T20:34:43Z

include/envoy/filesystem/filesystem.h

+
+  /**
+   * @return string a human-readable string describing the error code
+   * TODO(sesmith177) Use the IOError class after #5829 merges


I'm going to merge this soon, so up to you if you want to merge that in or just do a quick follow up here.

we've got a follow-up queued up; we will fix this there

…ssLogFile; add generic File object (envoyproxy#5772) The RawFile + RawInstance classes will be used to hold platform-specific filesystem code. The File/Instance classes will be used to add stats collecting and periodic flushing -- they will be platform agnostic. This is step 2/3 in adding Windows filesystem support. Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Sophie Wigmore <swigmore@pivotal.io> Signed-off-by: Fred Douglas <fredlas@google.com>

sesmith177 added 2 commits January 30, 2019 12:27

fix typo

62f8086

Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123 assigned jmarantz Jan 31, 2019

sesmith177 added 4 commits February 4, 2019 09:34

Merge branch 'master' into filesystem-decorator

6ba486c

Signed-off-by: Sam Smith <sesmith177@gmail.com>

add CREATE + RDWR to spelling dictionary

1f291bf

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-decorator

43e5665

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-decorator

3614b45

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Feb 5, 2019

View reviewed changes

sesmith177 and others added 2 commits February 5, 2019 14:58

Merge branch 'master' into filesystem-decorator

f50508a

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

Address feedback

779da9c

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

jmarantz reviewed Feb 5, 2019

View reviewed changes

jmarantz previously approved these changes Feb 5, 2019

View reviewed changes

jmarantz mentioned this pull request Feb 5, 2019

IoHandle interface #5829

Closed

add todo

1b22c96

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

sesmith177 dismissed jmarantz’s stale review via 1b22c96 February 5, 2019 21:45

jmarantz previously approved these changes Feb 5, 2019

View reviewed changes

mattklein123 requested changes Feb 6, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 6, 2019

mattklein123 self-assigned this Feb 6, 2019

jmarantz reviewed Feb 6, 2019

View reviewed changes

typedef -> using

ac7d4c1

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 dismissed jmarantz’s stale review via ac7d4c1 February 6, 2019 14:09

repokitteh-read-only bot removed the waiting label Feb 6, 2019

sesmith177 added 3 commits February 6, 2019 16:26

wip

a2c8c4f

Signed-off-by: Sam Smith <sesmith177@gmail.com> wip Signed-off-by: Sam Smith <sesmith177@gmail.com> wip Signed-off-by: Sam Smith <sesmith177@gmail.com>

wip

443a17a

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-decorator

8f48f6c

Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123 added the waiting label Feb 7, 2019

rename File -> AccessLogFile and RawFile -> File

5db0857

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 requested a review from lizan as a code owner February 7, 2019 19:06

repokitteh-read-only bot added the waiting label Feb 9, 2019

jmarantz reviewed Feb 10, 2019

View reviewed changes

sesmith177 added 2 commits February 11, 2019 08:39

Merge branch 'master' into filesystem-decorator

7e219e7

Signed-off-by: Sam Smith <sesmith177@gmail.com>

update todo

05862aa

Signed-off-by: Sam Smith <sesmith177@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 11, 2019

add Stats::Store& back to Api ctor

1b62da4

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

sesmith177 mentioned this pull request Feb 13, 2019

filesystem: add Windows implementation #5470

Closed

Merge branch 'master' into filesystem-decorator

4b8fbb3

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Feb 13, 2019

View reviewed changes

sesmith177 and others added 2 commits February 13, 2019 09:38

Merge branch 'master' into filesystem-decorator

91c161c

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Yael Harel <yharel@pivotal.io>

Revert createApiForTest

f8e155d

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz previously approved these changes Feb 15, 2019

View reviewed changes

mattklein123 added the waiting label Feb 17, 2019

Yael Harel and others added 2 commits February 21, 2019 08:49

Merge branch 'master' into filesystem-decorator

685a541

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

only close open files

85458c4

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 dismissed jmarantz’s stale review via 85458c4 February 21, 2019 13:51

repokitteh-read-only bot removed the waiting label Feb 21, 2019

Yael Harel added 2 commits February 21, 2019 13:39

Merge branch 'master' into filesystem-decorator

b973d12

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into filesystem-decorator

7db72ea

Signed-off-by: Yael Harel <yharel@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123 approved these changes Feb 21, 2019

View reviewed changes

mattklein123 merged commit 8aad39c into envoyproxy:master Feb 21, 2019


		typedef std::unique_ptr<RawFile> RawFilePtr;

		class RawInstance {

Conversation

sesmith177 commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sesmith177 commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sesmith177 commented Feb 7, 2019

Uh oh!

mattklein123 commented Feb 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sesmith177 commented Jan 30, 2019 •

edited

Loading

jmarantz Feb 5, 2019 •

edited

Loading