- Notifications
You must be signed in to change notification settings - Fork36
FUSE SUPPORT#184
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
FUSE SUPPORT#184
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ca2bf88 to032f142Comparetestgres/node.py Outdated
| defdata_dir(self): | ||
| # NOTE: we can't run initdb without user's args | ||
| returnos.path.join(self.base_dir,DATA_DIR) | ||
| ifhasattr(self,'_data_dir'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
как-то путанно выходит, может через DATA_DIR, только проверять абсолютный или относительный путь там указан? Есть абсолютный, то не подставлять base_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Не, я создаю просто пустую директорию куда буду монтировать fuse и хочу потом сменить data_dir у ноды на нее, чтобы можно было ноду запустить с новой директорией.
У директория куда монтируется не обязательно называется data.
Какая разница.
dmitry-lipetskFeb 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Привет.
Я с подозрением отношусь к этой фиче со сменой директории у ноды. Ноду нужно сразу создать с указанием нужной директории и после этого не давать её менять. То, что это (как я понимаю) сейчас невозможно - это другой вопрос.
У нас есть base_dir. От неё мы "наследуем" data_dir и log_dir.
Нужно иметь возможность явно указать эти data_dir и base_dir. Это будет уже совсем другой тип ноды, у которой нет base_dir.
По хорошему тут нужен другой вариант, который, в упрощенной форме, выглядит так:
- Нужно соорудить базовый класс для PostgresNode - PostgresNodeBase.
- У PostgresNodeBase будет два "абстрактных" свойства - data_dir и log_dir.
- Туда же, в PostgresNodeBase, перенесем функционал PostgresNode.
- Текущий PostgresNode наследует этот PostgresNodeBase и завязывает data_dir и log_dir на base_dir
- Вариант ноды, в которой можно указывать data_dir и log_dir делаем другим классом. Скажем - PostgresNode2. В его конструктор будут передаваться явные значения для data_dir и log_dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Как идея может просто тогда давать возможность задавать DataDir при конструкции ноды?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
можно, но тогда нужно custom_data_dir задавать, чтобы не конфликтовало с def data_dir(self). а вот в data_dir определять в зависимости от того задан custom_data_dir или нет.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@demonolock@dmitry-lipetsk
Подумал и переделал это в корне по другому. Теперь можно указать при старте ноды и ее остановке custom_data_dir.
Так ок?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@demonolock@dmitry-lipetsk убрал сеттер вовсе, обошелся без него пустой нодой. Таки сообразил как. Вы правы. Аппрув?
| defrun(self,command,gdb=False,old_binary=False,return_id=True,env=None, | ||
| skip_log_directory=False,expect_error=False,use_backup_dir=True): | ||
| skip_log_directory=False,expect_error=False,use_backup_dir=True,daemonize=False): | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Это будет раз на команду задаваться или на весь тестовый прогон?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Раз на команду, нужно для того чтобы мочь запустить команду в фоновом режиме. В моем случае чтобы запустить пробэкап с опцией fuse в фоне.
| logging.info(f"Process started in background with PID:{self.process.pid}") | ||
| ifself.process.stdoutandself.process.stderr: | ||
| stdout_thread=threading.Thread(target=stream_output,args=(self.process.stdout,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
, daemon=True?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Да, кажется разумно, поправлю.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Поправлено
| stderr=subprocess.STDOUT, | ||
| env=env | ||
| ).decode('utf-8',errors='replace') | ||
| ifdaemonize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
часть в daemonize можно вынести в функцию например такую form_daemon_process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Сделаю, разумно
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Поправлено
| ifdaemonize: | ||
| defstream_output(stream:subprocess.PIPE)->None: | ||
| forlineiniter(stream.readline,''): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
что если в цикле ошибка будет? Кажется нужен try - finally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Разумно, сделаю
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Поправлено
testgres/node.py Outdated
| returnself | ||
| defstop(self,params=[],wait=True): | ||
| defstop(self,params=[],wait=True,custom_data_dir=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Может быть проблема при внезапной остановке теста. В местах с вызовом stop(), например _try_shutdown. На cleanup еще пожалуйста посмотри, как он должен отработать, обе директории custom_data_dir и data_dir зачищать или только одну?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Хм, а как быть? Сохранять custom_data_dir в self и в try shutdown смотреть и убивать?
Вот если бы не в with force, то он просто бы по пиду убивал и все..
cleanup я бы оставил на откуп внешнего пользователя..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Как быть - не знаю.
Но выглядит так, как будто код пошел вразнос ))
dmitry-lipetskMar 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Думаю, что все таки надо будет реализовывать ту идею, которую я предложил с самого начала.
Там еще под раздачу попадает os_ops.
В текущем варианте реализована своеобразная логика выбора os_ops для PostgresNode у которой есть проблемы.
Насчет data_dir. Посмотрите#222. Я пока не добавлял производный класс с поддержкой явных data_dir и logs_dir. |
| @property | ||
| defdata_dir(self): | ||
| # NOTE: we can't run initdb without user's args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Верни пожалуйста комментарий
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Поправил
81a5eb4 intomasterUh oh!
There was an error while loading.Please reload this page.
No description provided.