Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option 2: Mono Repository with Umbrella Apps Structure #472

Merged
merged 484 commits into from
May 27, 2022
Merged

Option 2: Mono Repository with Umbrella Apps Structure #472

merged 484 commits into from
May 27, 2022

Conversation

albsch
Copy link
Member

@albsch albsch commented May 10, 2022

Included local dependencies vectorclock, antidote-erlang-client, antidote_crdt, antidote_pb_codec, antidote_stats.

+

  • Shared top-level configuration (common linting rules, dialyzer rules, etc.)
  • Implicit local code dependency management

-

  • Duplication of configurations in rebar3.config
    • e.g. C compiling directives need to be in the top-level and antidote rebar.config, if one wants to compile antidote in the stand-alone app directory

Other comments:

  • single shared _build folder
  • Commands are executed at the top-level
  • Local dependencies are not locked to a version

Changes:

  • bumped rebar3 version

  • default vm.args until benchmarks prove otherwise

  • uniform linting for umbrella project

  • fixed linting errors

  • updated rebar3_lint plugin

  • unified test coverage

  • erlang client removed from antidote dependencies

  • erlang client can be included as a standalone dependency in other projects
    (path dependency)

  • single format run with erlfmt

    • fixed comments which were moved one scope outside with replace
    • From: ^(\s*)%(.*\s*)\n(\s*)\{\n
    • To: $3\{\n $1% $2\n
  • added erlfmt --check to workflow

  • added git history of local apps as unrelated merged git histories

  • rebar3 dialyzer reports two errors:

    apps/antidote/src/clocksi_readitem.erl
    Line 86 Column 13: The pattern {'error', Reason} can never match the type {'ok','valid' | {'invalid',_,binary()}}

    apps/antidote/src/materializer_vnode.erl
    Line 387 Column 17: The pattern 'ignore' can never match the type #{_=>pos_integer()}

Breaking changes:

  • antidote_crdt
    • lint: antidote_crdt_counter_b:localPermissions/2 renamed to antidote_crdt_counter_b:local-permissions/2
      • Affects antidote bcounter_mgr

Todos future (create issues):

Todos for this PR:

Checked (top-level)

  • rebar3 deps
  • rebar3 eunit (+coverage)
  • rebar3 proper (+coverage)
  • rebar3 lint
  • rebar3 dialyzer
  • rebar3 xref
  • rebar3 release
  • reltest
  • ct system tests
  • Makefile
  • Github workflows
  • local Docker image
  • local stats endpoint producing data (localhost:3001/metrics)
  • Prometheus and Grafana docker setup
  • Test erlang client in other project
  • Code coverage

@albsch albsch added the dependency Issues related to dependencies of AntidoteDB label May 19, 2022
@bieniusa bieniusa self-requested a review May 20, 2022 08:59
@define-null
Copy link
Contributor

define-null commented May 20, 2022

@albsch Based on commit 7d772b1:
This fix eliminates dialyzer warnings for me. Spec correction to take into account ignore, but also {error, no_snapshot} is not correct.

diff --git a/apps/antidote/src/materializer_vnode.erl b/apps/antidote/src/materializer_vnode.erl
index 24c5dcce..21f88a21 100644
--- a/apps/antidote/src/materializer_vnode.erl
+++ b/apps/antidote/src/materializer_vnode.erl
@@ -377,7 +377,7 @@ internal_read(Key, Type, MinSnapshotTime, TxId, _PropertyList, ShouldGc, State)
                                 clocksi_readitem:read_property_list(),
                                 boolean(), state()) -> {ok, valid}
                                                         | {ok, {invalid, snapshot(), object_token()}}
-                                                        | {error, no_snapshot}.
+                                                        | {error, term()}.
 internal_validate_or_read(Key, Type, Token, MinSnapshotTime, TxId, _PropertyList, ShouldGc, State) ->
     SnapshotGetResp = get_from_snapshot_cache(TxId, Key, Type, MinSnapshotTime, State),

@@ -488,7 +488,7 @@ fetch_updates_from_cache(OpsCache, Key) ->
     end.

 -spec materialize_snapshot(txid() | ignore, key(), type(), snapshot_time(), boolean(), state(), snapshot_get_response())
-    -> {ok, {snapshot_time(), snapshot()}} | {error, reason()}.
+    -> {ok, {snapshot_time() | ignore, snapshot()}} | {error, term()}.

 materialize_snapshot(_TxId, _Key, _Type, _MinSnapshotTime, _ShouldGC, _State, #snapshot_get_response{
             number_of_ops=0,

@define-null
Copy link
Contributor

define-null commented May 20, 2022

@albsch @bieniusa
Taking into account that we import a lot of "new code" here there are 2 suggestions:

  • consider full history import instead of the single version, so that we notice if we introduce any accidental changes to the original code
  • add necessary format corrections, to fix indentation, long lines, tab/space issues, so that to avoid doing that work in the future and avoiding "touching too much" commits.

@bieniusa
Copy link
Contributor

Regarding the formatter:

  • Using the rebar3_format plugin with the otp_formatter, it adds quotes to the macros in the binary types in inter_dc_utils and breaks compilations.
  • The default version works :)

@albsch
Copy link
Member Author

albsch commented May 27, 2022

Thanks @bieniusa and @define-null for helping! Going to merge the PR now.

@albsch albsch merged commit 32a89d4 into AntidoteDB:master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-style dependency Issues related to dependencies of AntidoteDB enhancement test&build Improving the build and test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.