Skip to content

Refactor the core Builder mechanism#8684

Merged
wu-sheng merged 7 commits intomasterfrom
refactor
Mar 16, 2022
Merged

Refactor the core Builder mechanism#8684
wu-sheng merged 7 commits intomasterfrom
refactor

Conversation

@wu-sheng
Copy link
Member

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.
  • Refactor the core Builder mechanism, new storage plugin could implement their own converter and get rid of the hard requirement of using HashMap to communicate between data object and database native structure.
  • [Breaking Change] Break all existing 3rd-party storage extensions.

@wu-sheng wu-sheng added core feature Core and important feature. Sometimes, break backwards compatibility. enhancement Enhancement on performance or codes labels Mar 15, 2022
@wu-sheng wu-sheng added this to the 9.0.0 milestone Mar 15, 2022
@wu-sheng
Copy link
Member Author

For making reviewers easier, here are the key points.

New StorageBuilder

/**
 * Converter between the give T and K.
 *
 * @param <T> A storage entity implementation.
 */
public interface StorageBuilder<T extends StorageData> {
    /**
     * Use the given converter to build an OAP entity object.
     *
     * @param converter to transfer data format
     * @return an OAP entity object
     */
    T storage2Entity(Convert2Entity converter);

    /**
     * Use the given converter to build a database preferred structure.
     *
     * @param entity    to be used
     * @param converter provides the converting logic and hosts the converted value. Use {@link
     *                  Convert2Storage#obtain()} to read the converted data.
     */
    void entity2Storage(T entity, Convert2Storage converter);
}

StorageBuilder wouldn't accept HashMap as a parameter or the return value, now Convert2Entity and Convert2Storage interfaces open the responsibility of this temporary value holder to the storage plugin implementations.

Default HashMapConverter

To keep all codes are changed smoothly, I introduced HashMapConverter as the default converter. @lujiajing1126 Such as BanyanDB plugin could use its native one, and use native structure and get rid of HashMap permanently.

public class HashMapConverter {
    /**
     * Stateful Hashmap based converter, build object from a HashMap type source.
     */
    @RequiredArgsConstructor
    public static class ToEntity implements Convert2Entity {
        private final Map<String, Object> source;

        @Override
        public Object get(final String fieldName) {
            return source.get(fieldName);
        }
    }

    /**
     * Stateful Hashmap based converter, from object to HashMap.
     */
    public static class ToStorage implements Convert2Storage<Map<String, Object>> {
        private Map<String, Object> source;

        public ToStorage() {
            source = new HashMap();
        }

        @Override
        public void accept(final String fieldName, final Object fieldValue) {
            source.put(fieldName, fieldValue);
        }

        @Override
        public Object get(final String fieldName) {
            return source.get(fieldName);
        }

        @Override
        public Map<String, Object> obtain() {
            return source;
        }
    }
}

This HashMapConverter is super easy, just as a wrapper of HashMap.


All other code changes are just following these two proposals, and making codes work as usual.

Notice, there could be more potential changes for JDBC and other storage plugins, such as wrap ResultSet as a new ToEntity, rather than ResultSet->HashMap->Entity

wankai123
wankai123 previously approved these changes Mar 15, 2022
Copy link
Member

@wankai123 wankai123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wu-sheng wu-sheng merged commit 4def159 into master Mar 16, 2022
@wu-sheng wu-sheng deleted the refactor branch March 16, 2022 00:22
@lujiajing1126
Copy link
Contributor

lujiajing1126 commented Mar 16, 2022

We're planning to migrate to this new API, my only concern is that the conversion between binary data and its base64 representation still exists in various entities.

So for BanyanDB (we directly store byte array), we still cannot fully take advantage of this refactor. Is that possible we move this conversion somewhere else? Or any sugguestion?

wu-sheng added a commit that referenced this pull request Mar 16, 2022
…8685)

* Remove the hard requirement of BASE64 encoding for the binary field, and resolved concerns from #8684 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core feature Core and important feature. Sometimes, break backwards compatibility. enhancement Enhancement on performance or codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants