[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

By popular demand, I've attached below a long explanation of Kudu's approach
to object initialization and thread safety that came up during code review.

Because we don't use exceptions within Kudu, the initialization of an object
is typically done in two phases:
 1. Constructor, for operations that cannot fail.
 2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status
type, and needn't be thread safe because it is called right after the
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new
object and one to initialize an object with existing state. The idea is that
callers always use the constructor and then call one of the two initializer
functions, depending on their use case. WritablePBContainerFile is one such
object; Init() is for a brand new container file, and Open() is for a
container file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer
function for files with existing state, and it was an arbitrary function
(like Append()) that could be called at any time and by any thread. That
second responsibility is why it was thread safe. Now, OpenExisting() is just
the equivalent of CreateNew(), and so it makes sense for it to be just as
thread unsafe as CreateNew() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the
LBM didn't _need_ it to be thread safe. But, I think it was net less
confusing for Reopen() to be thread safe (and thus equivalent in semantics
to Append()) than for it to be thread unsafe (and thus exceptional when
compared to Append(), Sync(), etc.).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Reviewed-on: http://gerrit.cloudera.org:8080/6825
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 33 insertions(+), 40 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Todd Lipcon: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 4: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6825

to look at the new patch set (#4).

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

By popular demand, I've attached below a long explanation of Kudu's approach
to object initialization and thread safety that came up during code review.

Because we don't use exceptions within Kudu, the initialization of an object
is typically done in two phases:
 1. Constructor, for operations that cannot fail.
 2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status
type, and needn't be thread safe because it is called right after the
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new
object and one to initialize an object with existing state. The idea is that
callers always use the constructor and then call one of the two initializer
functions, depending on their use case. WritablePBContainerFile is one such
object; Init() is for a brand new container file, and Open() is for a
container file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer
function for files with existing state, and it was an arbitrary function
(like Append()) that could be called at any time and by any thread. That
second responsibility is why it was thread safe. Now, OpenExisting() is just
the equivalent of CreateNew(), and so it makes sense for it to be just as
thread unsafe as CreateNew() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the
LBM didn't _need_ it to be thread safe. But, I think it was net less
confusing for Reopen() to be thread safe (and thus equivalent in semantics
to Append()) than for it to be thread unsafe (and thus exceptional when
compared to Append(), Sync(), etc.).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 33 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/6825/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2193: Status LogBlockManager::OpenContainerMetadataWriter(
> kinda funny that this is removed again in the next patch. was that a patch-
I found the conflict resolution challenging and wanted a quick out. But, it is 
pretty silly. Let me fix the order and republish.


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2193: Status LogBlockManager::OpenContainerMetadataWriter(
kinda funny that this is removed again in the next patch. was that a 
patch-splitting gone awry or on purpose?


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6825

to look at the new patch set (#3).

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

By popular demand, I've attached below a long explanation of Kudu's approach
to object initialization and thread safety that came up during code review.

Because we don't use exceptions within Kudu, the initialization of an object
is typically done in two phases:
 1. Constructor, for operations that cannot fail.
 2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status
type, and needn't be thread safe because it is called right after the
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new
object and one to initialize an object with existing state. The idea is that
callers always use the constructor and then call one of the two initializer
functions, depending on their use case. WritablePBContainerFile is one such
object; Init() is for a brand new container file, and Open() is for a
container file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer
function for files with existing state, and it was an arbitrary function
(like Append()) that could be called at any time and by any thread. That
second responsibility is why it was thread safe. Now, OpenExisting() is just
the equivalent of CreateNew(), and so it makes sense for it to be just as
thread unsafe as CreateNew() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the
LBM didn't _need_ it to be thread safe. But, I think it was net less
confusing for Reopen() to be thread safe (and thus equivalent in semantics
to Append()) than for it to be thread unsafe (and thus exceptional when
compared to Append(), Sync(), etc.).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 65 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/6825/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 2:

(and as long as you're writing the walls of text anyway might as well put them 
front and center, for posterity)

-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 2:

gotta meet expectations

-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_);
 :   RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), _, 
_));
 :   ContainerSupHeaderPB sup_header;
 :   RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, 
_, _header));
 :   RETURN_NOT_OK(writer_->Size(_)); // Reset the write 
offset to the end of the file.
 :   state_ = FileState::OPEN;
> nice writeup, could you copy paste this to the commit message?
Sure why not. JD told me me people already expect walls of text from me anyway 
and I hate to disappoint.


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_);
 :   RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), _, 
_));
 :   ContainerSupHeaderPB sup_header;
 :   RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, 
_, _header));
 :   RETURN_NOT_OK(writer_->Size(_)); // Reset the write 
offset to the end of the file.
 :   state_ = FileState::OPEN;
> (I'm sure you're already aware of most of this long explanation, but I'm le
nice writeup, could you copy paste this to the commit message?


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2201: w.reset(new WritablePBContainerFile(std::move(f)));
> semi-unrelated question: now that we've had the file cache for a while and 
Sure, will do it in a follow-on patch.


http://gerrit.cloudera.org:8080/#/c/6825/2/src/kudu/util/pb_util.h
File src/kudu/util/pb_util.h:

PS2, Line 280: Init
> maybe best to rename this to OpenNew and the one below OpenForAppend or Ope
OK, I'll use CreateNew() and OpenExisting().


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 2201:   if (file_cache_) {
semi-unrelated question: now that we've had the file cache for a while and it 
seems stable, can we start just assuming it's always on and reduce some of 
these branches? Especially considering we don't test the "no cache" case, seems 
like cruft we can remove


http://gerrit.cloudera.org:8080/#/c/6825/2/src/kudu/util/pb_util.h
File src/kudu/util/pb_util.h:

PS2, Line 280: Init
maybe best to rename this to OpenNew and the one below OpenForAppend or 
OpenExisting? Despite the nice comments, I found the names a bit confusing.


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello David Ribeiro Alves, Todd Lipcon, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6825

to look at the new patch set (#2).

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 53 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/6825/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

PS1, Line 292: OpenContainerMetadataWriter
> docs
Done


http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_);
 :   RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), _, 
_));
 :   ContainerSupHeaderPB sup_header;
 :   RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, 
_, _header));
 :   RETURN_NOT_OK(writer_->Size(_)); // Reset the write 
offset to the end of the file.
 :   state_ = FileState::OPEN;
> why was this thread safe before and what changed that it doesn't need to be
(I'm sure you're already aware of most of this long explanation, but I'm 
leaving it intact for the benefit of others)

Because we don't use exceptions within Kudu, the initialization of an object is 
typically done in two phases:
1. Constructor, for operations that cannot fail.
2. Initializer function, for operations that may fail.

In most objects, the initializer function is named Init(), returns a Status 
type, and needn't be thread safe because it is called right after the 
constructor, before the object is "made public" to other threads.

Some objects offer two initializer functions: one to initialize a brand new 
object and one to initialize an object with existing state. The idea is that 
callers always use the constructor and then call one of the two initializer 
functions, depending on their use case. WritablePBContainerFile is one such 
object; Init() is for a brand new container file, and Open() is for a container 
file that already exists on disk.

Previously, Reopen() served double duty: it was both the initializer function 
for files with existing state, and it was an arbitrary function (like Append()) 
that could be called at any time and by any thread. That second responsibility 
is why it was thread safe. Now, Open() is just the equivalent of Init(), and so 
it makes sense for it to be just as thread unsafe as Init() is.

To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the LBM 
didn't _need_ it to be thread safe. But, I think it was net less confusing for 
Reopen() to be thread safe (and thus equivalent in semantics to Append()) than 
for it to be thread unsafe (and thus exceptional when compared to Append(), 
Sync(), etc.).


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

PS1, Line 292: OpenContainerMetadataWriter
docs


http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_);
 :   RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), _, 
_));
 :   ContainerSupHeaderPB sup_header;
 :   RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, 
_, _header));
 :   RETURN_NOT_OK(writer_->Size(_)); // Reset the write 
offset to the end of the file.
 :   state_ = FileState::OPEN;
why was this thread safe before and what changed that it doesn't need to be so 
anymore?


-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: reopen container metadata writers at the OS level

[Adar Dembo (Code Review)]

Hello David Ribeiro Alves, Todd Lipcon,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/6825

to review the following change.

Change subject: log block manager: reopen container metadata writers at the OS 
level
..

log block manager: reopen container metadata writers at the OS level

Another patch I'm working on compacts LBM metadata files at startup by
writing compacted metadata into a temporary file, then overwriting the
existing metadata file with it. For this to work properly, "reopening" a
metadata file should actually reopen the file on the filesystem. This patch
does just that.

There should be no impact on any existing code; the reopening done after
truncating a partial record from a metadata file is just as happy if done at
the logical level (before) as at the physical level (now).

Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
---
M src/kudu/fs/log_block_manager-test-util.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
6 files changed, 50 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/6825/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon