Add world save mode for ServerWorld#save() to support flush

[Lignium]

SpongeAPI | Sponge

[Zidane]

Now explain exactly what the two enum values do :). Rather what the implementation is contractually obligated to do.

[ImMorpheus]

I'm not sure about the naming.

world.save(SAVE) is a bit confusing. What about ENQUEUE / ENQUEUE_AND_FLUSH ?
Input wanted

[aromaa]

I think this is fine as-is. Most I/O operations have the pattern that they eventually flush unless you explicitly call to do so. I don't think its worth it to make the name more explicit on how the save is performed.

[ImMorpheus]

Most I/O operations have the pattern that they eventually flush unless you explicitly call to do so.

Yep, that was used under the hood by the previous method: you only had save() and whether there was any flushing/queue was an implementation detail.

We opted to move that detail to the API and allow users to decide whether to flush or just "queue" a save. My concern with this is that the SAVE option is misleading.
One could argue that a user would expect world.save(SAVE) to actually save the world, thinking that world.save(QUEUE) was an option. (The opposite of the current implementation).

[aromaa]

I'm speaking about in general how I/O operations are actually implemented in Java or in other languages as an API. When you open a handle and try write to it, it doesn't actually do anything with it yet but buffers it. You can write more and you might not see anything on disk. But eventually, something might show up unless the program crashes in the middle and you may explicitly call flush and those changes are immediately available.

So in my opinion this is something I expect how these types of APIs work. You do the operation and optionally flush for immediate results.

[ImMorpheus]

I'm speaking about in general how I/O operations are actually implemented in Java or in other languages as an API. When you open a handle and try write to it, it doesn't actually do anything with it yet but buffers it. You can write more and you might not see anything on disk. But eventually, something might show up unless the program crashes in the middle and you may explicitly call flush and those changes are immediately available.

I know but my point was that users of the API did not have any expectations of the method flushing or queueing the save. If we introduce a save mode and people have to deal with decision fatigue because it is not obvious what SAVE means in world.save(SAVE) then I think we are reducing the usability of the API.

To be clear, I don't mind having an option to flush in the API, I just think it should be better communicated to the user. IMO even keeping the old save with no guarantee and adding a saveAndFlush that flushes to disk would be better.

[aromaa]

Ah, I see. Looks like I misunderstood the last part. I can agree on that the SAVE option could be misleading and developers could now expect it to mean save and flush. Maybe we could make the enum names more explicit? SAVE_WORLD, SAVE_METADATA, SAVE_CHUNKS. Not really my favorite but it would not be as ambiguous?

Or perhaps the best way is to actually just introduce new method, saveAndFlush?

[ImMorpheus]

More explicit enums also work. I think that and saveAndFlush are both improvements compared to the current status 👍

[aromaa]

LGTM. Unless anyone has any objections, I'm willing to take this in.

[aromaa]

Some concerns were raised about whatever the new method is ambiguous with the old one and that it would be a bit better to be more explicit here. The first option would be to introduce a new method saveAndFlush and keep the old one. The second option would be to use more explicit enum names.

I'm not really sure what would be the best approach for the enums here and would vision it more like:

public enum WorldSaveMode {
    SAVE_CHUNKS,
    SAVE_METADATA,
    FLUSH
}

boolean save(EnumSet<WorldSaveMode> saveModes) throws IOException;

This would give more flexibility but it could hurt more the typical use case as you usually would want to have (SAVE_CHUNKS, SAVE_METADATA) combo and optionally add the FLUSH option there. Also does passing just FLUSH mean its a no-op or does it flush the pending writes if there are any? On top of that it does require more stuff on the implementation side. In my opinion using enums here would look cumbersome without the use of EnumSet.

Sorry for going back and forth with this.

[Lignium]

Some concerns were raised about whatever the new method is ambiguous with the old one and that it would be a bit better to be more explicit here. The first option would be to introduce a new method saveAndFlush and keep the old one. The second option would be to use more explicit enum names.

I like the option with the saveAndFlush() method. It's simple and obvious. Better than the boolean flag. Initially, I did so, but Zidane expressed an opinion about the Enum option. Other potential "modes" of saving in the future, as it seems to me, cannot be for the sake of which we need to do Enum. And the SerializationBehavior is responsible for managing what needs to be stored (manually or automatic), it is actually a world parameter, not a passed argument. I suggest just making the saveAndFlush() method. If in the future you need to redo this, the method can be easily deprecated and redirect the call to the right place.

[aromaa]

Other potential "modes" of saving in the future, as it seems to me, cannot be for the sake of which we need to do Enum.

I would imagine you could want to do something like world.save(EnumSet.of(WorldSaveMode.SAVE_METADATA)) to save the metadata immediately but without the chunks. But it is more complicated.

I suggest just making the saveAndFlush() method.

That should be enough for now to get this merged.