Core: Reset to main as part of replace shouldn't remove main ref #11819
Conversation
amogh-jahagirdar
force-pushed
the
fix-reset-main-for-rest
branch
2 times, most recently
from
6c4287d to
94e317b
Compare
amogh-jahagirdar
force-pushed
the
fix-reset-main-for-rest
branch
from
94e317b to
621b7af
Compare
| if (refName.equals(SnapshotRef.MAIN_BRANCH)) { | ||
| metadataBuilder.resetMainBranch(); | ||
| } else { | ||
| metadataBuilder.removeRef(refName); | ||
| } |
There was a problem hiding this comment.
Yeah at this point we may as well make TableMetadata.removeRef not clear the log if it's main, and just have the consistent behavior for resetting to main. I feel like that's the sane default for that API now but the only issue is if others depend on that behavior
Yeah sorry it's possibly a way to solve #11777, sorry I didn't quite mark it yet with "fixes" as I'm still mulling through the tradeoffs and wanted to see if CI broke with this change. @ebyhr what do you think? I was largely trying to think of simple ways to avoid a protocol change , and making the "RemoveRef(Main)" update not clear the snapshot history seemed to be the best way. There's the TableMetadata.removeRef API which will just clear the snapshot log , and we may just want to either change the behavior of that/add a separate removeRefAndHistory API for use cases which require clearing the snapshot log. |
RemoveRef(main) will clear history, we don't want to do that during a replace.