Hadoop Common
  1. Hadoop Common
  2. HADOOP-8230

Enable sync by default and disable append

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.1.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Append is not supported in Hadoop 1.x. Please upgrade to 2.x if you need append. If you enabled dfs.support.append for HBase, you're OK, as durable sync (why HBase required dfs.support.append) is now enabled by default. If you really need the previous functionality, to turn on the append functionality set the flag "dfs.support.broken.append" to true.
      Show
      Append is not supported in Hadoop 1.x. Please upgrade to 2.x if you need append. If you enabled dfs.support.append for HBase, you're OK, as durable sync (why HBase required dfs.support.append) is now enabled by default. If you really need the previous functionality, to turn on the append functionality set the flag "dfs.support.broken.append" to true.

      Description

      Per HDFS-3120 for 1.x let's:

      • Always enable the sync path, which is currently only enabled if dfs.support.append is set
      • Remove the dfs.support.append configuration option. We'll keep the code paths though in case we ever fix append on branch-1, in which case we can add the config option back
      1. hadoop-8230.txt
        21 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          Patch attached.

          test-patch is clean, I'm running the full unit test suite now, and will also do some cluster testing for sanity.

          Show
          Eli Collins added a comment - Patch attached. test-patch is clean, I'm running the full unit test suite now, and will also do some cluster testing for sanity.
          Hide
          Todd Lipcon added a comment -

          +1, looks good to me.

          Show
          Todd Lipcon added a comment - +1, looks good to me.
          Hide
          Eli Collins added a comment -

          I've run the full unit test suite and there were no new additional failures.

          Show
          Eli Collins added a comment - I've run the full unit test suite and there were no new additional failures.
          Hide
          Eli Collins added a comment -
               [exec] 
               [exec] -1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 4 new or modified tests.
               [exec] 
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
               [exec] 
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
               [exec] 
               [exec]     -1 findbugs.  The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings.
               [exec] 
          

          8 findbugs are HADOOP-7847.

          Show
          Eli Collins added a comment - [exec] [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 4 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] -1 findbugs. The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings. [exec] 8 findbugs are HADOOP-7847 .
          Hide
          Eli Collins added a comment -

          I've committed this. Thanks for the review Todd!

          Show
          Eli Collins added a comment - I've committed this. Thanks for the review Todd!
          Hide
          Suresh Srinivas added a comment -

          Eli, sorry for the late comment. I agree with the general direction of splitting hflush/hsync feature from append. Perhaps these features should be using two different flags.

          I have concerns with this change:

          1. I thought the proposal from HDFS-3120 was to add "dfs.support.sync". I do not see that flag in this patch.
          2. There are installations where hsync/hflush is disabled, using dfs.support.append. That option should be preserved.
          3. "dfs.support.broken.append" - why add this and not delete the tests that are testing append functionality?
          Show
          Suresh Srinivas added a comment - Eli, sorry for the late comment. I agree with the general direction of splitting hflush/hsync feature from append. Perhaps these features should be using two different flags. I have concerns with this change: I thought the proposal from HDFS-3120 was to add "dfs.support.sync". I do not see that flag in this patch. There are installations where hsync/hflush is disabled, using dfs.support.append. That option should be preserved. "dfs.support.broken.append" - why add this and not delete the tests that are testing append functionality?
          Hide
          Eli Collins added a comment - - edited

          Thanks for chiming in Suresh.

          Wrt #1 see this comment in HDFS-3120 that outlines the proposal that Todd, Nicholas and I thought was best. Feel free to file a follow-on jira for an improvement, happy to review. I'll update the description to match the proposal.

          Wrt #2 personally I don't think we should allow people to disable durable sync as that can result in data loss for people running HBase. See HADOOP-8230 for more info. I'm open to having an option to disable durable sync if you think that use case is important.

          Wrt #3 the rationale was two-fold: (1) there are tests that are using append not to test append per se but for the side effects and we'd lose sync test coverage by removing those tests and (2) per the description we're keeping the append code path in case someone wants to fix the data loss issues in which case it makes sense to keep the test coverage as well.

          Show
          Eli Collins added a comment - - edited Thanks for chiming in Suresh. Wrt #1 see this comment in HDFS-3120 that outlines the proposal that Todd, Nicholas and I thought was best. Feel free to file a follow-on jira for an improvement, happy to review. I'll update the description to match the proposal. Wrt #2 personally I don't think we should allow people to disable durable sync as that can result in data loss for people running HBase. See HADOOP-8230 for more info. I'm open to having an option to disable durable sync if you think that use case is important. Wrt #3 the rationale was two-fold: (1) there are tests that are using append not to test append per se but for the side effects and we'd lose sync test coverage by removing those tests and (2) per the description we're keeping the append code path in case someone wants to fix the data loss issues in which case it makes sense to keep the test coverage as well.
          Hide
          Suresh Srinivas added a comment -

          Wrt #2 personally I don't think we should allow people to disable durable sync as that can result in data loss for people running HBase. See HADOOP-8230 for more info. I'm open to having an option to disable durable sync if you think that use case is important.

          There are installations where HBase is not used and sync was disabled. Now this patch has removed that option. When an installation upgrades to a release with this patch, suddenly sync is enabled and there is no way to disable it.

          (1) there are tests that are using append not to test append per se but for the side effects and we'd lose sync test coverage by removing those tests and (2) per the description we're keeping the append code path in case someone wants to fix the data loss issues in which case it makes sense to keep the test coverage as well.

          For testing sync, with this patch, since it is enabled by default, you do not need the flag right?

          Show
          Suresh Srinivas added a comment - Wrt #2 personally I don't think we should allow people to disable durable sync as that can result in data loss for people running HBase. See HADOOP-8230 for more info. I'm open to having an option to disable durable sync if you think that use case is important. There are installations where HBase is not used and sync was disabled. Now this patch has removed that option. When an installation upgrades to a release with this patch, suddenly sync is enabled and there is no way to disable it. (1) there are tests that are using append not to test append per se but for the side effects and we'd lose sync test coverage by removing those tests and (2) per the description we're keeping the append code path in case someone wants to fix the data loss issues in which case it makes sense to keep the test coverage as well. For testing sync, with this patch, since it is enabled by default, you do not need the flag right?
          Hide
          stack added a comment -

          When an installation upgrades to a release with this patch, suddenly sync is enabled and there is no way to disable it.

          Would such an installation be using the sync call?

          Show
          stack added a comment - When an installation upgrades to a release with this patch, suddenly sync is enabled and there is no way to disable it. Would such an installation be using the sync call?
          Hide
          Eli Collins added a comment -

          For testing sync, with this patch, since it is enabled by default, you do not need the flag right?

          Correct, after my patch the tests that no longer use append no longer set the append flag. The tests that call append to get its side effects still use the append flag.

          Agree w Stack wrt the previous comment. Making sync actually work is a bug fix, it was a bug that we allowed people to call sync and unlike append there wasn't a flag to enable it that was disabled by default. Better to fix the default behavior (which allows you to sync).

          Show
          Eli Collins added a comment - For testing sync, with this patch, since it is enabled by default, you do not need the flag right? Correct, after my patch the tests that no longer use append no longer set the append flag. The tests that call append to get its side effects still use the append flag. Agree w Stack wrt the previous comment. Making sync actually work is a bug fix, it was a bug that we allowed people to call sync and unlike append there wasn't a flag to enable it that was disabled by default. Better to fix the default behavior (which allows you to sync).
          Hide
          Suresh Srinivas added a comment -

          Would such an installation be using the sync call?

          No from what I know.

          From what I understand, the intention of this change is to:

          1. Disable append, since 1.x has bugs in that implementation.
          2. Enable sync by default.

          Making sync actually work is a bug fix, it was a bug that we allowed people to call sync and unlike append there wasn't a flag to enable it that was disabled by default. Better to fix the default behavior (which allows you to sync).

          The implementation earlier used dfs.supports.append to support both durable sync and append. When this flag is off, whole bunch of code got turned off, related to sync functionality on how the blocks are stored, block reports etc. Now with this change, this code can no longer be turned off. I agree with enabling sync by default. However, for folks who chose not to enable the related code and not impacted by it, we need to add a flag to turn off that functionality.

          Show
          Suresh Srinivas added a comment - Would such an installation be using the sync call? No from what I know. From what I understand, the intention of this change is to: Disable append, since 1.x has bugs in that implementation. Enable sync by default. Making sync actually work is a bug fix, it was a bug that we allowed people to call sync and unlike append there wasn't a flag to enable it that was disabled by default. Better to fix the default behavior (which allows you to sync). The implementation earlier used dfs.supports.append to support both durable sync and append. When this flag is off, whole bunch of code got turned off, related to sync functionality on how the blocks are stored, block reports etc. Now with this change, this code can no longer be turned off. I agree with enabling sync by default. However, for folks who chose not to enable the related code and not impacted by it, we need to add a flag to turn off that functionality.
          Hide
          Sanjay Radia added a comment -

          The sync fixes were a large number of individual fixes. They added risk for users that were not using the sync feature. Hence Sync was kept off by default for such users - this was a very conscious decision.
          Sync should be left off by default with an option to turn it on. This is an incompatible change.

          Show
          Sanjay Radia added a comment - The sync fixes were a large number of individual fixes. They added risk for users that were not using the sync feature. Hence Sync was kept off by default for such users - this was a very conscious decision. Sync should be left off by default with an option to turn it on. This is an incompatible change.
          Hide
          Todd Lipcon added a comment -

          Couldn't the same be said for any new feature? Given that sync was fixed prior to the 1.0 release, I don't see why this should be considered an incompatible change. Many much bigger incompatible changes went into 1.0 when compared to earlier 0.20.20x or 0.20.x releases (e.g tarball layout entirely changed, for example). This doesn't affect any of the existing APIs, only the underlying implementations.

          Given this is committed for 1.1, not 1.0.x, I don't buy the reasoning that it's too much risk. We've had sync on by default in hundreds of our customer clusters for almost two years now and have yet to see a related data-loss event. The only bugs we've seen have been bugs where sync() wouldn't provide the correct semantics, but for installs which don't use sync, that doesn't matter.

          Show
          Todd Lipcon added a comment - Couldn't the same be said for any new feature? Given that sync was fixed prior to the 1.0 release, I don't see why this should be considered an incompatible change. Many much bigger incompatible changes went into 1.0 when compared to earlier 0.20.20x or 0.20.x releases (e.g tarball layout entirely changed, for example). This doesn't affect any of the existing APIs, only the underlying implementations. Given this is committed for 1.1, not 1.0.x, I don't buy the reasoning that it's too much risk. We've had sync on by default in hundreds of our customer clusters for almost two years now and have yet to see a related data-loss event. The only bugs we've seen have been bugs where sync() wouldn't provide the correct semantics, but for installs which don't use sync, that doesn't matter.
          Hide
          Suresh Srinivas added a comment -

          We've had sync on by default in hundreds of our customer clusters for almost two years now and have yet to see a related data-loss event. The only bugs we've seen have been bugs where sync() wouldn't provide the correct semantics, but for installs which don't use sync, that doesn't matter.

          That is great.

          Still, I think we should retain ability to turn it off, because I want to continue running my installation that way and this patch removes that ability.

          Show
          Suresh Srinivas added a comment - We've had sync on by default in hundreds of our customer clusters for almost two years now and have yet to see a related data-loss event. The only bugs we've seen have been bugs where sync() wouldn't provide the correct semantics, but for installs which don't use sync, that doesn't matter. That is great. Still, I think we should retain ability to turn it off, because I want to continue running my installation that way and this patch removes that ability.
          Hide
          Todd Lipcon added a comment -

          Seems a reasonable compromise is to instate a dfs.support.sync flag, but set it to true by default. That way those who are nervous about the bug fix can disable it, but the average user who can have HBase/etc working without additional configuration.

          Show
          Todd Lipcon added a comment - Seems a reasonable compromise is to instate a dfs.support.sync flag, but set it to true by default. That way those who are nervous about the bug fix can disable it, but the average user who can have HBase/etc working without additional configuration.
          Hide
          Suresh Srinivas added a comment -

          Adding dfs.support.sync flag is along the lines of my previous comments. I am reluctantly okay with enabling it by default. This should be a blocker on 1.1. It might be easy to revert this patch, and add the new flag, as lot of paths to be enabled by the new flag are removed in this patch.

          Show
          Suresh Srinivas added a comment - Adding dfs.support.sync flag is along the lines of my previous comments. I am reluctantly okay with enabling it by default. This should be a blocker on 1.1. It might be easy to revert this patch, and add the new flag, as lot of paths to be enabled by the new flag are removed in this patch.
          Hide
          Sanjay Radia added a comment -

          > Couldn't the same be said for any new feature? Given that sync was fixed prior to the 1.0 release,
          > I don't see why this should be considered an incompatible change.
          This was turned on in the 20 originally and then we had to turn it off due bugs.
          Given that the default in Hadoop 1 is off, why not leave it off and give a way to turn it on.
          The current default is off and I don't see a reason to change that default in 1.1.
          There are installations have that are using the current default.
          Can you please explain the reason the make this change?

          Show
          Sanjay Radia added a comment - > Couldn't the same be said for any new feature? Given that sync was fixed prior to the 1.0 release, > I don't see why this should be considered an incompatible change. This was turned on in the 20 originally and then we had to turn it off due bugs. Given that the default in Hadoop 1 is off, why not leave it off and give a way to turn it on. The current default is off and I don't see a reason to change that default in 1.1. There are installations have that are using the current default. Can you please explain the reason the make this change?
          Hide
          Todd Lipcon added a comment -

          This was turned on in the 20 originally and then we had to turn it off due bugs.

          Then several us spent many months fixing those bugs, and we haven't seen any since.

          Given that the default in Hadoop 1 is off, why not leave it off and give a way to turn it on.
          The current default is off and I don't see a reason to change that default in 1.1.
          There are installations have that are using the current default.
          Can you please explain the reason the make this change?

          It's a pain for HBase users to have to manually flip this, and risk data loss if they don't. Changing the default also means we have fewer code paths to maintain for the average user.

          Show
          Todd Lipcon added a comment - This was turned on in the 20 originally and then we had to turn it off due bugs. Then several us spent many months fixing those bugs, and we haven't seen any since. Given that the default in Hadoop 1 is off, why not leave it off and give a way to turn it on. The current default is off and I don't see a reason to change that default in 1.1. There are installations have that are using the current default. Can you please explain the reason the make this change? It's a pain for HBase users to have to manually flip this, and risk data loss if they don't. Changing the default also means we have fewer code paths to maintain for the average user.
          Hide
          Eli Collins added a comment -

          Suresh, Sanjay, the rationale is discussed in HDFS-3120. In short:

          1. We shouldn't provide a flag that enables append because we know append has data loss issues
          2. HBase and other programs have data loss when running against a default Hadoop 1.x install

          The rationale is pretty clear - this prevents data loss.

          Per my earlier comment, I'm open to having an option to disable durable sync if you think that use case is important, but what is that use case? Given that the sync code path is well tested and debugged, why would you want to run with a buggy sync implementation?

          Show
          Eli Collins added a comment - Suresh, Sanjay, the rationale is discussed in HDFS-3120 . In short: We shouldn't provide a flag that enables append because we know append has data loss issues HBase and other programs have data loss when running against a default Hadoop 1.x install The rationale is pretty clear - this prevents data loss. Per my earlier comment, I'm open to having an option to disable durable sync if you think that use case is important, but what is that use case? Given that the sync code path is well tested and debugged, why would you want to run with a buggy sync implementation?
          Hide
          Koji Noguchi added a comment -

          , but what is that use case?

          I'm probably a minority here but I do want the "an option to disable durable sync" not because it could be buggy but because it could be too stable compared to 0.23/2.0. Before moving all of our non-HBase clusters to 2.0, we might use 1.1 for some time. During this period, I do not want some production projects to start relying on the sync features then find some regression/difference on 2.0 blocking our upgrade schedule.

          Show
          Koji Noguchi added a comment - , but what is that use case? I'm probably a minority here but I do want the "an option to disable durable sync" not because it could be buggy but because it could be too stable compared to 0.23/2.0. Before moving all of our non-HBase clusters to 2.0, we might use 1.1 for some time. During this period, I do not want some production projects to start relying on the sync features then find some regression/difference on 2.0 blocking our upgrade schedule.
          Hide
          stack added a comment -

          This was turned on in the 20 originally and then we had to turn it off due bugs.

          The "bugs" were fixed a good while ago.

          Can you please explain the reason the make this change?

          + HBase needs it.
          + Its broken that users have to flip a configuration flag, then stop+start, to make a basic fs api method work.

          Show
          stack added a comment - This was turned on in the 20 originally and then we had to turn it off due bugs. The "bugs" were fixed a good while ago. Can you please explain the reason the make this change? + HBase needs it. + Its broken that users have to flip a configuration flag, then stop+start, to make a basic fs api method work.
          Hide
          Suresh Srinivas added a comment -

          what is that use case?

          I think I have explained it in the comments above. To repeat:

          "Still, I think we should retain ability to turn it off, because I want to continue running my installation that way and this patch removes that ability."

          Show
          Suresh Srinivas added a comment - what is that use case? I think I have explained it in the comments above. To repeat: "Still, I think we should retain ability to turn it off, because I want to continue running my installation that way and this patch removes that ability."
          Hide
          Eli Collins added a comment -

          @Suresh, I understand that someone said they want to run their installation w/ broken sync the question is why they want to run an installation with broken sync. I filed HADOOP-8365 for such a flag here, but there's currently no rationale for why you'd want to do that. Also, someone choosing to upgrade from 1.0.x to 1.1 is going to pick up new changes - and even new features - and most of them don't have a flag to disable them. Nothing new there.

          @Koji, per your comment..

          Before moving all of our non-HBase clusters to 2.0, we might use 1.1 for some time. During this period, I do not want some production projects to start relying on the sync features then find some regression/difference on 2.0 blocking our upgrade schedule.

          There may be a misunderstanding: the dfs.support.append flag never controlled whether sync was enabled. DFSClient#sync and NN#fsync have always been available - enabling sync by default does not expose any new APIs to your users that were not previously available. The difference is that this fixes bugs for your users that were already using sync, which I think you'll want.

          Show
          Eli Collins added a comment - @Suresh, I understand that someone said they want to run their installation w/ broken sync the question is why they want to run an installation with broken sync. I filed HADOOP-8365 for such a flag here, but there's currently no rationale for why you'd want to do that. Also, someone choosing to upgrade from 1.0.x to 1.1 is going to pick up new changes - and even new features - and most of them don't have a flag to disable them. Nothing new there. @Koji, per your comment.. Before moving all of our non-HBase clusters to 2.0, we might use 1.1 for some time. During this period, I do not want some production projects to start relying on the sync features then find some regression/difference on 2.0 blocking our upgrade schedule. There may be a misunderstanding: the dfs.support.append flag never controlled whether sync was enabled. DFSClient#sync and NN#fsync have always been available - enabling sync by default does not expose any new APIs to your users that were not previously available . The difference is that this fixes bugs for your users that were already using sync, which I think you'll want.
          Hide
          Suresh Srinivas added a comment -

          There may be a misunderstanding: the dfs.support.append flag never controlled whether sync was enabled.

          dfs.support.append turned off some code paths. These code paths are not just related to append. They enable durable sync. See the patch where it changes, "if support append then do x else do y" to do "x" without any check. That is the behavior I want a user to be able to turn off with a flag.

          Show
          Suresh Srinivas added a comment - There may be a misunderstanding: the dfs.support.append flag never controlled whether sync was enabled. dfs.support.append turned off some code paths. These code paths are not just related to append. They enable durable sync. See the patch where it changes, "if support append then do x else do y" to do "x" without any check. That is the behavior I want a user to be able to turn off with a flag.
          Hide
          Eli Collins added a comment -

          I get that, what I'm saying is that I don't see the rationale for disabling the durable sync code paths. I'm -0 on HADOOP-8365, if you feel strongly that we should have a config option that let's people keep the previous/broken sync behavior go for it. I just don't see it.

          Show
          Eli Collins added a comment - I get that, what I'm saying is that I don't see the rationale for disabling the durable sync code paths. I'm -0 on HADOOP-8365 , if you feel strongly that we should have a config option that let's people keep the previous/broken sync behavior go for it. I just don't see it.
          Hide
          stack added a comment -

          That is the behavior I want a user to be able to turn off with a flag.

          Do you think there many users who'd want to do this Suresh? I'd think the number few and if there any still conscious this option even exists, they are probably suffering from the FUD that sync is buggy/broke. We should help them get over their misconception? (Pardon me if I am way off on this. Just offering an opinion from outer-left-field)

          Show
          stack added a comment - That is the behavior I want a user to be able to turn off with a flag. Do you think there many users who'd want to do this Suresh? I'd think the number few and if there any still conscious this option even exists, they are probably suffering from the FUD that sync is buggy/broke. We should help them get over their misconception? (Pardon me if I am way off on this. Just offering an opinion from outer-left-field)
          Hide
          Suresh Srinivas added a comment -

          Do you think there many users who'd want to do this Suresh?

          There are several clusters that I support that do not use sync, that currently runs with append turned off.

          I'd think the number few and if there any still conscious this option even exists, they are probably suffering from the FUD that sync is buggy/broke. We should help them get over their misconception?

          I agree that the code that is being enabled has been stable for some time, which is the main reason why it was ported to 0.20.205. However I would like to retain the existing behavior and not enable a change unnecessarily on these clusters. This avoids having to worry about or spend time looking at any bugs/changed behavior that might crop up.

          For these kinds of changes (see several token related changes that happened in 1.x), I have always advocated adding a flag so existing deployments can stay unaffected. I am asking the same here. It is more important given this patch removed an option that existed to turn off new code.

          if you feel strongly that we should have a config option that let's people keep the previous/broken sync behavior go for it

          The need for an option is a comment on the patch committed in this jira. Sorry I could not comment quickly enough, as this patch was committed with a short turn around time. I think it should be addressed as a subsequent patch for this jira and not a separate optional item. Alternatively we could revert this change and rework it to add a flag.

          Show
          Suresh Srinivas added a comment - Do you think there many users who'd want to do this Suresh? There are several clusters that I support that do not use sync, that currently runs with append turned off. I'd think the number few and if there any still conscious this option even exists, they are probably suffering from the FUD that sync is buggy/broke. We should help them get over their misconception? I agree that the code that is being enabled has been stable for some time, which is the main reason why it was ported to 0.20.205. However I would like to retain the existing behavior and not enable a change unnecessarily on these clusters. This avoids having to worry about or spend time looking at any bugs/changed behavior that might crop up. For these kinds of changes (see several token related changes that happened in 1.x), I have always advocated adding a flag so existing deployments can stay unaffected. I am asking the same here. It is more important given this patch removed an option that existed to turn off new code. if you feel strongly that we should have a config option that let's people keep the previous/broken sync behavior go for it The need for an option is a comment on the patch committed in this jira. Sorry I could not comment quickly enough, as this patch was committed with a short turn around time. I think it should be addressed as a subsequent patch for this jira and not a separate optional item. Alternatively we could revert this change and rework it to add a flag.
          Hide
          Eli Collins added a comment -

          This change was proposed and discussed over a month ago:
          https://issues.apache.org/jira/browse/HDFS-3120?focusedCommentId=13241903&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13241903

          Since you're worried that the sync path being buggy I think the best remedy is testing. The sync path in 1.0.x has been tested extensively by HBase users (including MR jobs), but we need to spend time looking at any bugs/changed behavior that might crop up as part of testing the Hadoop 1.x release anyway.

          This change prevents known data loss on out of the box Hadoop 1.x installs, that seems more important than issues that could potentially come up during testing. I think our HBase users feel similarly. The reason I filed a separate jira for adding the flag is that I don't think that we should let users enable a code path that we know can result in data loss, and means we have to test two code paths. I also see your POV which is why I'm not -1 on this flag even though I don't like it.

          Show
          Eli Collins added a comment - This change was proposed and discussed over a month ago: https://issues.apache.org/jira/browse/HDFS-3120?focusedCommentId=13241903&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13241903 Since you're worried that the sync path being buggy I think the best remedy is testing. The sync path in 1.0.x has been tested extensively by HBase users (including MR jobs), but we need to spend time looking at any bugs/changed behavior that might crop up as part of testing the Hadoop 1.x release anyway. This change prevents known data loss on out of the box Hadoop 1.x installs, that seems more important than issues that could potentially come up during testing. I think our HBase users feel similarly. The reason I filed a separate jira for adding the flag is that I don't think that we should let users enable a code path that we know can result in data loss, and means we have to test two code paths. I also see your POV which is why I'm not -1 on this flag even though I don't like it.
          Hide
          Suresh Srinivas added a comment -

          I had marked HADOOP-8365 as a blocker for 1.1.0.

          Since HADOOP-8365 has not been fixed yet for 1.1.0, I am -1 on this patch. If HADOOP-8365 gets fixed, I will remove my -1.

          Show
          Suresh Srinivas added a comment - I had marked HADOOP-8365 as a blocker for 1.1.0. Since HADOOP-8365 has not been fixed yet for 1.1.0, I am -1 on this patch. If HADOOP-8365 gets fixed, I will remove my -1.
          Hide
          Eli Collins added a comment -

          Patch for HADOOP-8365 coming, please review when you get a sec.

          Show
          Eli Collins added a comment - Patch for HADOOP-8365 coming, please review when you get a sec.
          Hide
          Matt Foley added a comment -

          Closed upon release of Hadoop-1.1.0.

          Show
          Matt Foley added a comment - Closed upon release of Hadoop-1.1.0.

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development