SAMZA-1768: Handle corrupted OFFSET file #588

xinyuiscool · 2018-07-27T20:51:40Z

This patch addresses the following tickets:

SAMZA-1778: SIGSEGV when reading properties (metrics) on a closed RocksDB store
SAMZA-1777: Logged store OFFSET file write during flush should be atomic
SAMZA-1768: Handle corrupted OFFSET file elegantly

xinyuiscool · 2018-07-27T20:52:21Z

@prateekm : could you please take a look?

prateekm · 2018-07-27T20:53:45Z

samza-core/src/main/java/org/apache/samza/storage/StorageManagerUtil.java

+      try {
+        offset = FileUtil.readWithChecksum(offsetFileRef);
+      } catch (Exception e) {
+        LOG.warn("Fail to read offset file of " + storePath, e);


Minor: For consistency: LOG.warn("Failed to read offset file in storage partition directory: {}", offsetFileName, storepath, e)

prateekm · 2018-07-27T20:58:02Z

samza-core/src/main/scala/org/apache/samza/util/FileUtil.scala

    }
+
+    //atomic swap of tmp and real offset file
+    Files.move(tmpFile.toPath, file.toPath, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING)


Do we need REPLACE_EXISTING if ATOMIC_MOVE is specified? From the javadoc:
ATOMIC_MOVE: The move is performed as an atomic file system operation and all other options are ignored. If the target file exists then it is implementation specific if the existing file is replaced or this method fails by throwing an IOException.

Also, this should be in the try block since this can throw exceptions.

The caller already handles the exception by logging the error and throw a runtime exception.

Right, but would be good to move to try so that oos and fos are closed in finally.

ok, sure, let me do that.

prateekm · 2018-07-27T21:01:34Z

samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala

      (configuredMetrics ++ rocksDbMetrics)
-        .foreach(property => metrics.newGauge(property, () => rocksDb.getProperty(property)))
+        .foreach(property => metrics.newGauge(property, () =>
+          if (rocksDb.isOwningHandle) {


Minor: Maybe add a comment explaining that isOwningHandle is false if the db is closed.

Makes sense. Added the comment.

prateekm

LGTM, thanks!

shanthoosh · 2018-07-28T00:44:00Z

samza-core/src/main/scala/org/apache/samza/util/FileUtil.scala

      oos.writeUTF(data)
+
+      //atomic swap of tmp and real offset file
+      Files.move(tmpFile.toPath, file.toPath, StandardCopyOption.ATOMIC_MOVE)


I think we need to add StandardCopyOption.REPLACE_EXISTING to the existing options to Files.move method. Files.move throws up FileAlreadyExistsException if target file already exists and we don't provide that Replace_existing option as an additional option.

Atomic move is a file system level primtiive operation and not all file system support it by default. File.move throws up AtomicMoveNotSupportedException if underlying file system doesn't support it. It would be better to handle it as well.

Please change if it makes sense(Above details are taken from Files.move javadoc).

I don't think we need both REPLACE_EXISTING and ATOMIC_MOVE, but we might need to catch IOException as well where we catch AtomicMoveNotSupportedException. From the javadoc for ATOMIC_MOVE:

If the target file exists then it is implementation specific if the existing file is replaced or this method fails by throwing an IOException.

shanthoosh · 2018-07-28T00:45:26Z

samza-core/src/main/java/org/apache/samza/storage/StorageManagerUtil.java

      LOG.info("Found offset file in storage partition directory: {}", storePath);
-      offset = FileUtil.readWithChecksum(offsetFileRef);
+      try {
+        offset = FileUtil.readWithChecksum(offsetFileRef);


I think it would be better to add a unit test for these changes.

Added the test for the case of offset already exist.

xinyuiscool · 2018-07-30T16:23:30Z

@prateekm : I move the Files.move() out of the try{} block again since we should close the streams before we do the file movement. This is to enforce the write to the offset file before move. The IOExceptions are handled from the callers so we don't need to catch them in this function. The current handling of failing the container looks ok to me since it should happen very rarely, as we haven't seen it happening in production.

prateekm · 2018-07-30T17:42:54Z

@xinyuiscool Makes sense to move this to its own try-catch block. It might still be better to handle IOException explicitly and revert to a normal move + replace existing. The IOException can happen if the implementation doesn't support replacing existing files as a part of the move. My concern is that this might fail on a particular platform consistently and we'll always end up rebootstrapping. Alternatively, if you've tested that it works on OSX and Linux, that'll be good enough for now.

Otherwise LGTM, feel free to merge this.

xinyuiscool · 2018-07-30T17:56:22Z

Good point on testing on both OSX and linux. I only tested on linux. Let me try OSX. Thanks.

sborya · 2018-07-31T02:00:18Z

samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala


      (configuredMetrics ++ rocksDbMetrics)
-        .foreach(property => metrics.newGauge(property, () => rocksDb.getProperty(property)))
+        .foreach(property => metrics.newGauge(property, () =>


what is this change do?

xinyuiscool added 2 commits July 26, 2018 11:14

SAMZA-1768: Handle corrupted OFFSET file elegantly

27e24e8

Minor changes

853f48a

prateekm requested changes Jul 27, 2018

View reviewed changes

Address pr comments

8bbf69d

prateekm approved these changes Jul 27, 2018

View reviewed changes

shanthoosh reviewed Jul 28, 2018

View reviewed changes

xinyuiscool added 2 commits July 27, 2018 17:51

Further tweak of logic

c068fce

Add unit test for case of offset file aleady exists

ee1cf4a

asfgit closed this in 01a1795 Jul 30, 2018

sborya reviewed Jul 31, 2018

View reviewed changes

rmatharu-zz mentioned this pull request Nov 22, 2019

SAMZA-2397: Updating gauge-val function on newGauge on same metric name #1223

Merged

SAMZA-1768: Handle corrupted OFFSET file #588

SAMZA-1768: Handle corrupted OFFSET file #588

Uh oh!

Conversation

xinyuiscool commented Jul 27, 2018

Uh oh!

xinyuiscool commented Jul 27, 2018

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!

prateekm 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!

xinyuiscool commented Jul 30, 2018

Uh oh!

prateekm commented Jul 30, 2018

Uh oh!

xinyuiscool commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants