Implemented a file logger that uses WritableFileWriter #5491

ggaurav28 · 2019-06-19T21:55:29Z

Summary:
Current PosixLogger performs IO operations using posix calls. Thus the
current implementation will not work for non-posix env. Created a new
logger class EnvLogger that uses env specific WritableFileWriter for IO operations.

Test Plan:
make check

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

Are we ready to replace the existing PosixLogger, or shall we keep both in the beginning?

ggaurav28 · 2019-06-20T17:11:51Z

Are we ready to replace the existing PosixLogger, or shall we keep both in the beginning?

It may be safer to add a gflag to control whether to use a posix logger or a generic logger. We can enable new logger in debug build (by default) and later when we have confidence we can enable the new logger in release build as well.
Thoughts?

siying · 2019-06-20T17:14:53Z

@ggaurav28 RocksDB (except tools) doesn't rely on gflag, and we don't require users to use gflag at all. I suggest we leave default as PosixLogger for now, and users can directly create EnvLogger for now if they want. Gradually, we can talk about replacement.

ggaurav28 · 2019-06-20T17:23:19Z

users can directly create E

Sounds good. I will make the changes and update the PR.

facebook-github-bot · 2019-06-20T18:53:55Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

We need to expose a factory function under include/rocksdb for users to be able to create such a logger.

siying

And how about make it the default logger for Env::NewLogger()?

siying

I'm a little bit worried that we are adding some code with 0 unit test coverage. Can we add a unit test and does some simple verification?

logging/env_logger.h

siying · 2019-06-20T21:16:50Z

logging/env_logger.h

+  void Logv(const char* format, va_list ap) override {
+    IOSTATS_TIMER_GUARD(logger_nanos);
+
+    const uint64_t thread_id = (*gettid_)();


We had a discussion about changing it to a more modern way of getting ID. Does STL thread ID works?

env.cc has this function. Leverage it in the EnvLogger class.

uint64_t Env::GetThreadID() const { std::hash<std::thread::id> hasher; return hasher(std::this_thread::get_id()); }

siying · 2019-06-21T18:27:15Z

logging/env_logger.h

+ private:
+  std::unique_ptr<WritableFileWriter> file_;
+  uint64_t (*gettid_)();       // Return the thread id for the current thread
+  mutable port::Mutex mutex_;  // Mutex to protect the shared variables below.


Why does mutex_ need to be tagged as mutable?

In this case mutex does not need to be mutable as there are no const method acquiring the mutex.

Mutex now needs to be mutable as it is being acquired in const member method GetLogFileSize().

facebook-github-bot · 2019-06-21T18:49:42Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-06-21T18:50:51Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

logging/env_logger.h

facebook-github-bot · 2019-06-21T20:37:33Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

env/env.cc

siying · 2019-06-24T23:46:13Z

logging/env_logger.h

+      if (now_micros - last_flush_micros_ >= flush_every_seconds_ * 1000000) {
+        FlushLocked();
+      }
+      mutex_.WriteUnlock();


Can we use a scoped lock instead?

If we use scoped lock, then lock will be held while delete is called at line#145
i.e.,
if (base != buffer) { delete[] base; }
Is it acceptable to hold the lock while memory is being freed?

logging/env_logger.h

siying · 2019-06-24T23:50:20Z

env/env.cc

+  *result = std::make_shared<EnvLogger>(std::move(writable_file), fname,
+                                        options, this);
+  return Status::OK();
+}


Maybe I missed something but I don't see this function is covered in unit test.

This function can not be directly used in the test as this is a virtual method. PosixEnv which is being used in the test has overridden this method.

As you suggested, i have created a new public helper method NewEnvLogger(). This method is now being called from test.

env/env.cc

facebook-github-bot · 2019-06-25T20:36:54Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-06-25T20:37:53Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2019-06-25T21:31:24Z

include/rocksdb/env.h

+  // EnvLogger) for storing informational messages. This method is public for
+  // testing purposes.
+  Status NewEnvLogger(const std::string& fname,
+                      std::shared_ptr<Logger>* result);


Can we move it to a standalone function, rather than a function of Env?

Made this as a static method in EnvLogger class.

I'm confused here. It is still Env::NewEnvLogger(). Did I miss anything?

facebook-github-bot · 2019-06-27T18:47:16Z

@ggaurav28 has updated the pull request. Re-import the pull request

siying · 2019-06-27T19:33:58Z

logging/env_logger.h

+  }
+
+  static Status NewEnvLogger(const std::string& fname, Env* env,
+                             std::shared_ptr<Logger>* result) {


I hope the function is made public under include/rocksdb/. Now users cannot call this function, and they cannot easily call Env::NewLogger() either.

Siying, Can you please let me know the file under the include/rocksdb where i should put this method? I will make the change accordingly.

For now, maybe env.h?

Ok. Moved it back to env.h. Please review it again.

What I see now is that it is back to a function of Env. The problem is that users have to create a Env class to use it, but it takes an Env as a parameter. To me it is confusing.

What I suggested is that, we need to keep the function publically available, which means under include/rocksdb, this is what users can see after we release the code. Also, it should not be a function of Env. So a workable solution is to create a standalone function in include/rocksdb/env.h.

I misunderstood your earlier comment. Made it a standalone function in env.h.
I have implemented it in env.cc as EnvLogger does not have a cc file. If you prefer, i can create a env_logger.cc file to hold the implementation of NewEnvLogger.

facebook-github-bot · 2019-07-01T06:02:32Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-07-01T06:04:23Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…perations Summary: Current PosixLogger performs IO operations using posix calls. Thus the current implementation will not work for non-posix env. Created a new logger class EnvLogger that uses WritableFileWriter for IO operations. Test Plan: make check Reviewers: Subscribers: Tasks:T45934141 Tags:

Summary: PosixLogger will be replaced with EnvLogger later. Test Plan: make check -j32

Function NewEnvLogger is now a static method of EnvLogger class.

facebook-github-bot · 2019-07-03T00:30:11Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2019-07-03T00:52:16Z

include/rocksdb/env.h

@@ -1563,4 +1565,10 @@ Status NewHdfsEnv(Env** hdfs_env, const std::string& fsname);
 // This is a factory method for TimedEnv defined in utilities/env_timed.cc.
 Env* NewTimedEnv(Env* base_env);

+// Returns an instance of logger that can be used for storing informational
+// messages.
+// This is a factory method for EnvLogger declared in logging/env_logging.h


I know we did it in the function above but it's not a good idea to mention files outside include/rocksdb here, because they may not be available to users when it is released.

Should i update the comment to say "This is a factory method to create a generic logger that uses env specific methods for logging operations"?

I will push the code. We can make the changes in the comment later in a separate PR.

facebook-github-bot · 2019-07-09T18:19:28Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-07-09T19:52:40Z

@ggaurav28 has updated the pull request. Re-import the pull request

facebook-github-bot

@ggaurav28 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-07-10T00:27:41Z

@ggaurav28 merged this pull request in 60d8b19.

facebook-github-bot · 2019-07-10T00:28:08Z

@ggaurav28 merged this pull request in 60d8b19.

Summary: Current PosixLogger performs IO operations using posix calls. Thus the current implementation will not work for non-posix env. Created a new logger class EnvLogger that uses env specific WritableFileWriter for IO operations. Pull Request resolved: facebook#5491 Test Plan: make check Differential Revision: D15909002 Pulled By: ggaurav28 fbshipit-source-id: 13a8105176e8e42db0c59798d48cb6a0dbccc965

Summary: Current PosixLogger performs IO operations using posix calls. Thus the current implementation will not work for non-posix env. Created a new logger class EnvLogger that uses env specific WritableFileWriter for IO operations. Pull Request resolved: facebook/rocksdb#5491 Test Plan: make check Differential Revision: D15909002 Pulled By: ggaurav28 fbshipit-source-id: 13a8105176e8e42db0c59798d48cb6a0dbccc965 Signed-off-by: Changlong Chen <levisonchen@live.cn>

facebook-github-bot added the CLA Signed label Jun 19, 2019

ggaurav28 marked this pull request as ready for review June 19, 2019 21:58

facebook-github-bot reviewed Jun 19, 2019

View reviewed changes

siying reviewed Jun 20, 2019

View reviewed changes

ggaurav28 force-pushed the generic_env_logger branch from 8b57163 to adeebd7 Compare June 20, 2019 18:53

facebook-github-bot reviewed Jun 20, 2019

View reviewed changes

siying reviewed Jun 20, 2019

View reviewed changes

siying reviewed Jun 21, 2019

View reviewed changes

ggaurav28 force-pushed the generic_env_logger branch from c9f2e8f to 74af120 Compare June 21, 2019 18:50

facebook-github-bot reviewed Jun 21, 2019

View reviewed changes

siying suggested changes Jun 21, 2019

View reviewed changes

logging/env_logger.h Outdated Show resolved Hide resolved

siying reviewed Jun 21, 2019

View reviewed changes

logging/env_logger.h Outdated Show resolved Hide resolved

facebook-github-bot reviewed Jun 21, 2019

View reviewed changes

siying reviewed Jun 24, 2019

View reviewed changes

ggaurav28 force-pushed the generic_env_logger branch from c6892e9 to 682c8ab Compare June 25, 2019 20:37

facebook-github-bot reviewed Jun 25, 2019

View reviewed changes

siying reviewed Jun 26, 2019

View reviewed changes

siying approved these changes Jun 27, 2019

View reviewed changes

ggaurav28 force-pushed the generic_env_logger branch from f9faf68 to be43291 Compare July 1, 2019 06:03

facebook-github-bot reviewed Jul 1, 2019

View reviewed changes

ggaurav28 added 8 commits July 2, 2019 16:21

Keep PosixLogger as the default logger.

9d645ee

Summary: PosixLogger will be replaced with EnvLogger later. Test Plan: make check -j32

Added unit test for EnvLogger class and addressed review comments.

da5202c

Addressed review comments.

09b6c18

Replaced RwMutex with Mutex and added helper method NewEnvLogger.

7b4408e

Moved the utility function to create EnvLogger out of Env class.

35e510a

Function NewEnvLogger is now a static method of EnvLogger class.

Moved method NewEnvLogger back to Env class.

4bfe6df

Made NewEnvLogger a standalone function in env.h

1991af9

ggaurav28 force-pushed the generic_env_logger branch from be43291 to 1991af9 Compare July 3, 2019 00:30

facebook-github-bot reviewed Jul 3, 2019

View reviewed changes

siying approved these changes Jul 3, 2019

View reviewed changes

Merge branch 'master' into generic_env_logger

f93a1b0

Merge branch 'master' into generic_env_logger

3eba5d2

facebook-github-bot reviewed Jul 9, 2019

View reviewed changes

facebook-github-bot closed this in 60d8b19 Jul 9, 2019

facebook-github-bot added the Merged label Jul 10, 2019

Implemented a file logger that uses WritableFileWriter #5491

Implemented a file logger that uses WritableFileWriter #5491

Conversation

ggaurav28 commented Jun 19, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

ggaurav28 commented Jun 20, 2019

siying commented Jun 20, 2019

ggaurav28 commented Jun 20, 2019

facebook-github-bot commented Jun 20, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggaurav28 Jun 21, 2019 • 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

facebook-github-bot commented Jun 21, 2019

facebook-github-bot commented Jun 21, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 21, 2019

facebook-github-bot 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

facebook-github-bot commented Jun 25, 2019

facebook-github-bot commented Jun 25, 2019

facebook-github-bot 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

facebook-github-bot commented Jun 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 1, 2019

facebook-github-bot commented Jul 1, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 3, 2019

facebook-github-bot 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

facebook-github-bot commented Jul 9, 2019

facebook-github-bot commented Jul 9, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 10, 2019

facebook-github-bot commented Jul 10, 2019

ggaurav28 commented Jun 19, 2019 •

edited

Loading

ggaurav28 Jun 21, 2019 •

edited

Loading