Byzanteam / postgrex_wal

1 stars 1 forks source link

refactor: decode_in_stream #26

Closed json2008 closed 1 year ago

json2008 commented 1 year ago
  1. postgrex_wal/messages/util,提升至:postgrex_wal/util, 放在messages目录下不合适。
  2. postgrex_wal/messages,仅存放各种message
  3. postgrex_wal/message.ex,postgrex_wal/messages/message.ex二个文件(重名且都存在decode/1,容易混淆,建议拆分成:postgrex_wal/gen_message.ex,postgrex_wal/messages/message.ex
  4. postgrex_wal/gen_message.ex,作为generic message模块存在,定义行为,以及通用的message辅助函数,如:stream_start?, stream_stop?(代理到:PostgrexWal.Util.GenMessage下)
  5. postgrex_wal/messages下的各个message,都去@behaviour GenMessage, (callbacks: decode/1, identifier/0)
  6. identifier的函数命名问题。event中的第一个字节,代表不同的含义,程序中,都是用key::8来代表,仅一个字节。建议用函数名:key_byte/0
json2008 commented 1 year ago

@fahchen

请抽空过一下这个,看是否合适?

fahchen commented 1 year ago
  1. 关于 gen_message 我觉得不合适,因为系统级别的用 gen 比较合适(比如 gen_server,gen_event, gen_statem,gentcp 等),这里只是 wal 的 message,所以用 gen 不合适。

  2. behaviour 还是用 Message 比较合适

这样呢

  1. PostgrexWal.Message: behaviour
  2. PostgrexWal.Message.{Begin, Commit}: message modules
  3. 消息的 utlis 放到 PostgrexWal.Message.Util,不是消息的就放到 PostgrexWal.Util 里面
json2008 commented 1 year ago

有2个小问题:

  1. 会存在 PostgrexWal.Message, PostgrexWal.Message.Message ?,感觉有点混淆、凌乱?会有2个 Message 模块存在。(嵌套层级不同)
  2. PostgrexWal.Message.UtilPostgrexWal.Util,这两个模块,一旦做alias后,都是Util,调用其中的函数时,不易区分是那个模块里的?
json2008 commented 1 year ago
  defp decode_wal(<<key::8, _rest::binary>> = payload, state) do
    alias PostgrexWal.Messages.Util

    # alternative 1

    in_stream? =
      cond do
        Util.stream_start?(key) ->
          state.in_stream? && Logger.error("stream flag consecutively true")
          true

        Util.stream_stop?(key) ->
          state.in_stream? || Logger.error("stream flag consecutively false")
          false

        true ->
          state.in_stream?
      end

    # alternative 2

    in_stream? =
      case key do
        StreamStart.identifier() ->
          state.in_stream? && Logger.error("stream flag consecutively true")
          true

        StreamStop.identifier() ->
          state.in_stream? || Logger.error("stream flag consecutively false")
          false

        _ ->
          state.in_stream?
      end

    payload =
      if in_stream? and Util.streamable?(key),
        do: {:in_stream, payload},
        else: payload

    {Util.decode(payload), %{state | in_stream?: in_stream?}}
  end
end
  1. stream_start/1, stream_stop/1,这两个函数仅在这个地方用一次?
  2. stream是否开始?是否结束?这样的认知,应该仅限于PgSource模块内部。不扩散。(那么就应该是:alternative 2)
  3. 如果每个Message模块,自身带有identifier/0,那基于同样的理由,就应该也自带有:streamable?/0,以表示其是否能在stream中出现?(但是从此代码片段中,可看出,实现有点困难,是先有鸡,还是先有蛋的问题?或是带来复杂度)。
fahchen commented 1 year ago

有2个小问题:

  1. 会存在 PostgrexWal.Message, PostgrexWal.Message.Message ?,感觉有点混淆、凌乱?会有2个 Message 模块存在。(嵌套层级不同)
  2. PostgrexWal.Message.UtilPostgrexWal.Util,这两个模块,一旦做alias后,都是Util,调用其中的函数时,不易区分是那个模块里的?
  1. 不会有 PostgrexWal.Message.Message,只会有 PostgrexWal.Message
  2. Message 里面不应该使用 PostgrexWal.Util,如果其他地方同时使用 PostgrexWal.UtilPostgrexWal.Message.Util 这两个,可以用 alias
fahchen commented 1 year ago
```elixir
  defp decode_wal(<<key::8, _rest::binary>> = payload, state) do
    alias PostgrexWal.Messages.Util

    # alternative 1

    in_stream? =
      cond do
        Util.stream_start?(key) ->
          state.in_stream? && Logger.error("stream flag consecutively true")
          true

        Util.stream_stop?(key) ->
          state.in_stream? || Logger.error("stream flag consecutively false")
          false

        true ->
          state.in_stream?
      end

    # alternative 2

    in_stream? =
      case key do
        StreamStart.identifier() ->
          state.in_stream? && Logger.error("stream flag consecutively true")
          true

        StreamStop.identifier() ->
          state.in_stream? || Logger.error("stream flag consecutively false")
          false

        _ ->
          state.in_stream?
      end

    payload =
      if in_stream? and Util.streamable?(key),
        do: {:in_stream, payload},
        else: payload

    {Util.decode(payload), %{state | in_stream?: in_stream?}}
  end
end

stream_start/1, stream_stop/1,这两个函数仅在这个地方用一次? stream是否开始?是否结束?这样的认知,应该仅限于PgSource模块内部。不扩散。(那么就应该是:alternative 2) 如果每个Message模块,自身带有identifier/0,那基于同样的理由,就应该也自带有:streamable?/0,以表示其是否能在stream中出现?(但是从此代码片段中,可看出,实现有点困难,是先有鸡,还是先有蛋的问题?或是带来复杂度)。

  1. 我也觉得 alternative 2 更合适一些
  2. streamable?/0 我觉得不用放到单个 message 里面,可以 stream 理解成一个行为抽象,stream 应用在那些 message 上面,它可以直接决定,而不是 message 去决定